Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(opentelemetry-plugin-pg): omit pg.values by default #174

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

sergioregueira
Copy link
Contributor

Which problem is this PR solving?

This PR solves issue #11, that reports the PostgreSQL plugin must not log pg.values by default because they may contain sensitive data such as user credentials.

Short description of the changes

  1. I fixed eslint warnings found in master.
  2. I refactored the PgClientExtended type to inherit directly from pgTypes.Client instead of intersect them all the time.
  3. I documented arguments of several methods of utils.ts to avoid generic types (...args: unknown[])
  4. I renamed PgPluginQueryConfig interface to NormalizedQueryConfig (based on the plugin) to avoid confusion.
  5. I changed implementation to log pg.values only when enhancedDatabaseReporting (standard property) flag is se to true.
  6. I implemented several unit tests that validate the behavior of that flag creating a new specific test files for utils.ts instead of duplicating the assertions for each valid call.

@sergioregueira sergioregueira requested a review from a team August 16, 2020 08:36
@codecov
Copy link

codecov bot commented Aug 16, 2020

Codecov Report

Merging #174 into master will increase coverage by 0.07%.
The diff coverage is 98.85%.

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   94.23%   94.30%   +0.07%     
==========================================
  Files          76       77       +1     
  Lines        3766     3830      +64     
  Branches      406      413       +7     
==========================================
+ Hits         3549     3612      +63     
- Misses        217      218       +1     
Impacted Files Coverage Δ
...ugins/node/opentelemetry-plugin-pg/test/pg.test.ts 94.37% <ø> (ø)
...ns/node/opentelemetry-plugin-pg/test/utils.test.ts 98.36% <98.36%> (ø)
plugins/node/opentelemetry-plugin-pg/src/pg.ts 91.66% <100.00%> (+0.28%) ⬆️
plugins/node/opentelemetry-plugin-pg/src/utils.ts 97.01% <100.00%> (+0.04%) ⬆️

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like great work thanks!

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@obecny obecny merged commit 6af1022 into open-telemetry:master Aug 18, 2020
@obecny obecny added the internal label Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants