-
Notifications
You must be signed in to change notification settings - Fork 282
Update GraphQL Recommended vs Opt-In requirements #3118
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
base: main
Are you sure you want to change the base?
Update GraphQL Recommended vs Opt-In requirements #3118
Conversation
|
|
|
Note you will have to change the model file, and then generate the update commands. The markdown is not to be modified manually. Also, consider adding a note to the opt_in explaining the rationaly. See an example here: semantic-conventions/model/http/spans.yaml Line 130 in 2e68756
|
9b4c12a to
05695a8
Compare
|
This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:
Such changes may be rejected or put on hold until a new SIG/project is established. Please refer to the Semantic Convention Areas |
Thanks! And sorry, I realized that just after I opened it and shortly after I signed the CLA, which was the first thing I wanted to tackle. I'm not sure why this PR was just auto-closed though — do you have suggestions on that @joaopgrassi ? |
|
@joaopgrassi It looks like some relatively new automation auto-closed this, so ping again on the above if you get a chance. I guess this needs |
|
This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:
Such changes may be rejected or put on hold until a new SIG/project is established. Please refer to the Semantic Convention Areas |
1 similar comment
|
This PR contains changes to area(s) that do not have an active SIG/project and will be auto-closed:
Such changes may be rejected or put on hold until a new SIG/project is established. Please refer to the Semantic Convention Areas |
| # your pull request title with [chore] or use the "Skip Changelog" label. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: enhancement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| change_type: enhancement | |
| change_type: breaking |
Right? because now it won't be recorded by default anymore.
Fixes #2985
Changes
Changes the firmness for
graphql.documentfromRecommendedtoOpt-In.Context
The
graphql.documentis user-inputted, often contains sensitive information, is potentially unbounded in length and is also high-cardinality in the same way as the existinggraphql.operation.name(which is already warned about).Put another way,
graphql.documentis a general liability to have listed asRecommendedwithout serious infrastructure considerations and needs. This makes it a better candidate for being anOpt-Inattribute. In our customer adoption of OpenTelemetry and GraphQL, we've found GraphQL customers following this configuration/instruction purely based on its naming in this (still, experimental) specification and not understanding the implications (or just not thinking them through).In most all cases, the lesser liability of
graphql.operation.name(The actual operation name) is sufficient, as in many GraphQL deployments there is a link between the two which can be correlated out of band (e.g., client code-bases, operation manifests, etc.). While the operation name isn't without its risk, but it's more likely to be dozens of bytes of a limited character set rather than dozens, hundreds or potentially thousands of kilobytes. In that regard, it's reasonable thatgraphql.operation.namebe left asRecommendedfor user experience (though the argument could easily be made that it should also beOpt-In).Merge requirement checklist
[chore]