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.

Sign up for GitHub

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

Jump to bottom

[asyncpg] Shouldn't capture query parameters by default #854

Merged
merged 7 commits into from
Jul 28, 2020
Merged

[asyncpg] Shouldn't capture query parameters by default #854

merged 7 commits into from
Jul 28, 2020

Conversation

thomasdesr
Copy link
Contributor

@thomasdesr thomasdesr commented Jun 25, 2020 •
edited
Loading

Including parameterized query parameters in the span's metadata probably shouldn't be the default since they often contain sensitive data (e.g. user session tokens, hashed user passwords, etc).

To fix that this PR:

  • Switches the default behavior for the asyncpg instrumentor to not capture the query parameters in its metadata
  • Updates the tests to ensure that ^ is working as expected

@thomasdesr thomasdesr requested a review from a team June 25, 2020 03:37
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 25, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@thomasdesr
Copy link
Contributor Author

I signed it

@thomasdesr thomasdesr changed the title [ext][asyncpg] We shouldn't capture query parameters by default [asyncpg] Shouldn't capture query parameters by default Jun 25, 2020
HiveTraum
HiveTraum reviewed Jun 25, 2020
View reviewed changes
Copy link
Contributor

@HiveTraum HiveTraum left a comment

Choose a reason for hiding this comment

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

Some notes and thoughts

ext/opentelemetry-ext-asyncpg/src/opentelemetry/ext/asyncpg/__init__.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-asyncpg/src/opentelemetry/ext/asyncpg/__init__.py Outdated Show resolved Hide resolved
@aabmass
Copy link
Member

aabmass commented Jun 25, 2020

Thanks for doing this, I think this default behavior is much better!

We should probably also do this for the other DB integrations, I'll open a separate issue. It looks like a few of them use the opentelemetry-ext-dbapi wrappers which has the problem, too:

opentelemetry-python/ext/opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py

Line 312 in f8e947c

span.set_attribute("db.statement.parameters", str(args[1]))

@aabmass aabmass mentioned this pull request Jun 25, 2020
Closed
@thomasdesr thomasdesr force-pushed the asyncpg-skip-params-by-default branch from 9bd26a2 to a72e0be Compare June 28, 2020 18:16
@thomasdesr
Copy link
Contributor Author

@HiveTraum @aabmass fixed what I'd broken, ptal! :D

@thomasdesr
Copy link
Contributor Author

Gentle poke for reviews :D

@thomasdesr
Copy link
Contributor Author

ping @aabmass @HiveTraum

HiveTraum
HiveTraum approved these changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@HiveTraum HiveTraum left a comment

Choose a reason for hiding this comment

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

approved

aabmass
aabmass approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

LGTM! I am in favor of these defaults :) but I am not an official reviewer

@majorgreys
Copy link
Contributor

@thomasdesr This PR has two changes that we can separate: disabling the setting of the db.statement.parameters attribute and another for masking. I think the former is a change we have agreement on. But the latter needs some discussion as this is a concern for all instrumentations. If that makes sense for a path forward, let's start an issue for how to support masking attributes.

@thomasdesr
Copy link
Contributor Author

thomasdesr commented Jul 16, 2020 •
edited
Loading

@majorgreys 👍 I like this plan. I'll pull the masking out of the PR this evening.

@thomasdesr thomasdesr force-pushed the asyncpg-skip-params-by-default branch from 9d6eadf to dad3b4c Compare July 17, 2020 17:41
@thomasdesr
Copy link
Contributor Author

@majorgreys fixed! ptal :D

I took a look at the circleci failures and I believe they're unrelated?

codeboten
codeboten approved these changes Jul 28, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍

@codeboten codeboten merged commit 7f74a89 into open-telemetry:master Jul 28, 2020
@thomasdesr
Copy link
Contributor Author

Thanks for the reviews!!

@thomasdesr thomasdesr deleted the asyncpg-skip-params-by-default branch July 28, 2020 16:00
@stschenk stschenk mentioned this pull request Oct 26, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@aabmass aabmass aabmass approved these changes

@HiveTraum HiveTraum HiveTraum approved these changes

@lzchen lzchen lzchen approved these changes

Assignees

@majorgreys majorgreys

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@thomasdesr @aabmass @majorgreys @codeboten @HiveTraum @lzchen