-
Notifications
You must be signed in to change notification settings - Fork 1k
Add ability to document configuration options in metadata.yaml #13742
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
Conversation
e538bd4 to
5b6f386
Compare
| javaagent: | ||
| - com.datastax.oss:java-driver-core:[4.0,4.4) | ||
| configurations: | ||
| - name: otel.instrumentation.common.db-statement-sanitizer.enabled |
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.
@trask should we have a framework specific flags for this in all frameworks? I think it was added for a few.
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.
yeah, I almost wish we didn't have the "common" variant, to make it harder for users to do the wrong thing (send sensitive data unexpectedly from some other db instrumentation)
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.
would it be worth me opening an issue to replace the common config flags, or at the very least start with this one? I plan on working through module to module to document the various configs, so I can track the common ones I find and replace the usages with framework specific ones as I go, if that's a direction we want to move in.
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.
ah, I only dislike this one common flag due to it broadly disabling sensitive data sanitization, the other common flags I like
I'm not sure I dislike it enough though to cause upgrade pain by removing it
| - name: otel.instrumentation.common.db-statement-sanitizer.enabled | ||
| description: Enables or disables statement sanitization for database queries. | ||
| default: 'true' | ||
| - name: otel.instrumentation.common.peer-service-mapping |
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.
Not related to this PR but I guess the peer service handling is a bit inconsistent. I think we are using it only for some of the instrumentations.
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.
do you think it's worth an effort to standardize the approach?
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.
I think we should strive for consistent behavior across instrumentations, but it is one of the many nice to have things.
Co-authored-by: Lauri Tulmin <[email protected]>
Co-authored-by: Lauri Tulmin <[email protected]>
Related to #13468
Current stats output: