-
Notifications
You must be signed in to change notification settings - Fork 598
Rename logs_level_enabled flag to spec_unstable_logs_enabled
#2291
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
Rename logs_level_enabled flag to spec_unstable_logs_enabled
#2291
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2291 +/- ##
=====================================
Coverage 79.4% 79.4%
=====================================
Files 122 122
Lines 20783 20783
=====================================
+ Hits 16504 16506 +2
+ Misses 4279 4277 -2 ☔ View full report in Codecov by Sentry. |
…elemetry-rust into experimental-log-level-flag
|
|
||
| [features] | ||
| logs_level_enabled = ["opentelemetry/logs_level_enabled"] | ||
| experimental_logs_level_enabled = ["opentelemetry/experimental_logs_level_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.
https://github.com/open-telemetry/opentelemetry-rust/pull/2291/files#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055R156 Based on the wording here, the experimental spec based things should be feature flagged under otel_unstable ? Do we want to follow that approach or modify it?
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.
Yes, I thought using otel_unstable. But I think not good to use single feature flag for all the features. Will revisit the wording in separate PR.
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.
How about:
experimental_<flag-name>:
- Use for features that are NOT in any specification
- These are purely experimental/exploratory features
- When the feature's implementation becomes stable, rename to
<flag-name>
unstable_<flag-name>:
- Use for features that ARE in specifications but either:
- The spec itself is still experimental, or
- The spec is stable but our implementation is incomplete
- Remove this feature flag entirely once the feature is fully implemented and stable
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.
renamed to unstable_logs_level_enabled based on above comment.
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.
the name logs_level_enabled is not very clear. enabled() takes more than level, so why specially mention level?
maybe unstable_logs_enabled is good enough.?
Also, I am wondering if we need separate flags for each feature or a single flag for all unstable spec ones? The previous discussion is here #1410 (comment)
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.
maybe unstable_logs_enabled is good enough.?
Agree unstable_logs_enabled seems better
Also, I am wondering if we need separate flags for each feature or a single flag for all unstable spec ones?
Yes, the agreement in above link was to have otel_unstable as a single feature flag to enable/disable all features for which
- specs is still experimental,
- implementation is stable as per the current specs (i.e, won't crash/panic, no perf issues)
- can have breaking changes if the specs changes.
which means that it would be safe to enable/disable such features together without affecting the other existing performance. logs_level_enabled does fall in this category.
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.
Thinking more, I feel this brings better clarity, with different flags for each feature. I can update the CONTRIBUTION.md guide is we agree.
logs_level_enabled flag to experimental_logs_level_enabledlogs_level_enabled flag to unstable_logs_level_enabled
logs_level_enabled flag to unstable_logs_level_enabledlogs_level_enabled flag to specs_unstable_logs_level_enabled
logs_level_enabled flag to specs_unstable_logs_level_enabledlogs_level_enabled flag to specs_unstable_logs_enabled
logs_level_enabled flag to specs_unstable_logs_enabledlogs_level_enabled flag to spec_unstable_logs_enabled
cijothomas
left a comment
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.
Thanks!
Changes
The flag was added when this feature was not there in specs, so named as
logs_level_enabled. However, now the feature is there in specs under Dev state, so renamed it tospec_unstable_logs_enabled.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes