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

Change url.query requirement level from recommended to conditionally_required (to match url.path) #112

Closed
trask opened this issue Jun 14, 2023 · 2 comments · Fixed by #118
Assignees

Comments

@trask
Copy link
Member

trask commented Jun 14, 2023

Spinning this out from #109 as a separate issue.

Summarizing the discussion:

Pro: this seems like a bug that slipped into open-telemetry/opentelemetry-specification#3355
Con: query is particularly prone to containing sensitive information

@trask
Copy link
Member Author

trask commented Jun 15, 2023

from @Oberon00's #109 (comment)

it seems OK to be "just" recommended. I think that would allow instrumentations to provide an opt-out option which REQUIRED technically doesn't.

this is an interesting point

@lmolkova lmolkova changed the title Change url.query requirement level from recommended to required (to match url.path) Change url.query requirement level from recommended to conditionally_required (to match url.path) Jun 15, 2023
@lmolkova
Copy link
Contributor

lmolkova commented Jun 15, 2023

when sanitizing sensitive information, would we rather redact the values (i.e. foo=REDACTED&bar=REDACTED) or drop the attribute completely?

My preference is to preserve parameter names, order, etc rather than dropping the whole attribute.

I assume if users write sanitization, they do it as a processor (now) and will take the query attribute, parse, and sanitize individual properties. If we provide a tracing configuration to sanitize arbitrary values, we'd do something similar.

TLDR:

Opting-out needs configuration and this configuration would rather be a regex or a callback to sanitize than a boolean flag (drop/preserve).

Also, I feel we don't need to solve this problem prior to stability: it should not be breaking to switch between recommended and conditionally_required: when available at any moment.

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

Successfully merging a pull request may close this issue.

3 participants