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

Semantic conventions for database should be explicit about parameters / placeholder values #711

Closed
remram44 opened this issue Oct 7, 2021 · 5 comments
Assignees

Comments

@remram44
Copy link

remram44 commented Oct 7, 2021

What are you trying to achieve?

I would like to be able to see parameter values for SQL queries. I was about to raise an issue against the SQLAlchemy instrumentation, but noticed no specific guidance in the database conventions page.

The specs do mandate that the statement should be captured, but that statement may or may not be complete, depending on whether placeholders are used.

Additional context.

The full path (= with parameter values) is captured for HTTP requests, e.g. the flask instrumentation (for example) captures:

"http.route": "/pet/<int:helloid>"  # placeholders
"http.target": "/pet/123"  # parameterized

However if that HTTP requests triggers a SQL requests, it will show up as :

"db.statement": "SELECT * FROM pets WHERE id = %(param_1)s"

This seems inconsistent.

In addition, parameter values are exposed in systems that don't use SQL syntax, and are shown explicitly in the Redis example in the docs ("HMSET myhash field1 'Hello' field2 'World") which increases confusion.

There might be security considerations (see also open-telemetry/oteps#100), but they are probably not fully mitigated as data not using placeholders will still appear in the trace (see also open-telemetry/opentelemetry-specification#1659).

I think in any case, there should be some guidance in the spec, even if it is the opposite recommendation to the one I would personally want. The current one does not mention placeholders at all, even though they seem commonly considered when making decisions in this bug tracker.

@iNikem
Copy link
Contributor

iNikem commented Oct 7, 2021

The database semantic conventions specifically say about db.statement:

The value may be sanitized to exclude sensitive information.

@remram44
Copy link
Author

remram44 commented Oct 7, 2021

The information I want to get at is not sensitive, I will leave the questions of sensitive data to open-telemetry/oteps#100. When investigating performance issues, I want to know which project took a long time to load, and I can't get that from the database spans (gotta correlate with logs which is not trivial).

@SergeyKanzhelev SergeyKanzhelev removed their assignment Feb 18, 2023
@jack-berg jack-berg transferred this issue from open-telemetry/opentelemetry-specification Feb 7, 2024
@trask
Copy link
Member

trask commented Feb 8, 2024

Closing, as we have another issue tracking the same now: #716

@trask trask closed this as completed Feb 8, 2024
@remram44
Copy link
Author

remram44 commented Feb 8, 2024

I'm not holding my breath. It would be nice if I could get the basic support required so I can use open-telemetry for a simple CRUD app.

@trask
Copy link
Member

trask commented Feb 8, 2024

hi @remram44! we've just kicked off a database semantic convention stability effort: https://github.com/open-telemetry/community/blob/main/projects/database-client-semconv.md

I can't promise whether this specific issue will be included in the initial stability, but we will definitely be considering it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

5 participants