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

Rename logs_level_enabled flag to spec_unstable_logs_enabled #2291

Merged
merged 9 commits into from
Nov 11, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Nov 8, 2024

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 to spec_unstable_logs_enabled.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Sorry, something went wrong.

@lalitb lalitb requested a review from a team as a code owner November 8, 2024 23:21

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.4%. Comparing base (0cc2cd5) to head (d7000f5).
Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

…elemetry-rust into experimental-log-level-flag
@@ -21,14 +21,14 @@ opentelemetry-semantic-conventions = { path = "../opentelemetry-semantic-convent
] }

[features]
logs_level_enabled = ["opentelemetry/logs_level_enabled"]
experimental_logs_level_enabled = ["opentelemetry/experimental_logs_level_enabled"]
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

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)

Copy link
Member Author

@lalitb lalitb Nov 10, 2024

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.

Copy link
Member Author

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.

lalitb and others added 2 commits November 8, 2024 15:57

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@lalitb lalitb changed the title Rename logs_level_enabled flag to experimental_logs_level_enabled Rename logs_level_enabled flag to unstable_logs_level_enabled Nov 9, 2024
@lalitb lalitb changed the title Rename logs_level_enabled flag to unstable_logs_level_enabled Rename logs_level_enabled flag to specs_unstable_logs_level_enabled Nov 11, 2024
@lalitb lalitb changed the title Rename logs_level_enabled flag to specs_unstable_logs_level_enabled Rename logs_level_enabled flag to specs_unstable_logs_enabled Nov 11, 2024
@lalitb lalitb changed the title Rename logs_level_enabled flag to specs_unstable_logs_enabled Rename logs_level_enabled flag to spec_unstable_logs_enabled Nov 11, 2024
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@cijothomas cijothomas merged commit b833118 into open-telemetry:main Nov 11, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants