-
Notifications
You must be signed in to change notification settings - Fork 940
Stabilize part of SDK spec for synchronous instrument Enabled #4787
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?
Conversation
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.
Pull request overview
This PR standardizes the specification language around eventual visibility of configuration changes for the Enabled method across all OpenTelemetry signals (traces, metrics, and logs). The changes ensure consistent wording and placement of visibility guarantees while clarifying that metrics specifications apply specifically to synchronous instruments.
- Generalized the visibility statement from referencing specific parameters to "any of these parameters"
- Added clarification that metrics
EnabledSDK specification applies to synchronous instruments - Moved and consolidated visibility guarantees to eliminate duplication
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| specification/trace/sdk.md | De-indented visibility statement in TracerConfig to apply to all parameters at section level |
| specification/metrics/sdk.md | Added visibility statement to MeterConfig, clarified synchronous instrument scope, added inline status annotation to first condition, and removed duplicate visibility text |
| CHANGELOG.md | Added changelog entry documenting the stabilization of part of the Enabled SDK for synchronous instruments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Is this something we should wait to include in the January's release? |
I think this one should be high confidence and low risk, I suggest that we merge. |
Per #4746 (review)
Side note. I also made sure that this wording used for all signals (and in the same place)
so that the spec is similar for all signals