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 Optional attribute requirement level to Opt-In #135

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

trask
Copy link
Member

@trask trask commented Feb 17, 2023

Based on suggestion by @arminru and others.

Needed before we mark attribute-requirement-level.md as stable (which is needed before we mark HTTP semantic conventions stable, see open-telemetry/opentelemetry-specification#3219)

@trask trask force-pushed the rename-optional-to-opt-in branch 4 times, most recently from f5cca8e to 85d73c9 Compare February 17, 2023 20:33
@trask trask marked this pull request as ready for review February 17, 2023 20:57
@trask trask requested a review from a team February 17, 2023 20:57
@trask trask requested a review from Oberon00 as a code owner February 17, 2023 20:57
@trask trask changed the title Rename Optional requirement level to Opt-In Rename Optional attribute requirement level to Opt-In Feb 17, 2023
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

+1

@yurishkuro
Copy link
Member

yurishkuro commented Feb 17, 2023

I like the name change, but did we do an analysis that all usages of the optional level are actually opt-in`? Because the two are not exactly equivalent:

  • optional is something that instrumentation may or may not support (fairly similar to recommended, but to lesser degree)
  • opt-in is something that instrumentation MUST require an active opt-in before enabling the attribute

Is it possible that we need to keep both concepts and just do a once-over review if some currently optional attrs should really be opt-in? if we only need opt-in (given that optional ~= recommended), then I don't think this is how many people may actually understood optional previously, and as a result the attrs may be proactively emitted by instrumentation without asking user to enable them.

@trask
Copy link
Member Author

trask commented Feb 17, 2023

I just sent a draft spec PR to explore that: open-telemetry/opentelemetry-specification#3228

Luckily, optional isn't used in many places yet, so only two I'm unsure about which meaning was intended, I left comments to try to get clarification on those two places.

@Oberon00
Copy link
Member

Oberon00 commented Feb 20, 2023

Because the two are not exactly equivalent

There is actually a definition of our "optional" requirement level that says it's "opt-in": https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/attribute-requirement-level.md#optional

Optional

Instrumentations SHOULD populate the attribute if and only if the user configures the instrumentation to do so. Instrumentation that doesn't support configuration MUST NOT populate Optional attributes.

opt-in was also the original name this was proposed with in open-telemetry/opentelemetry-specification#2522 by @lmolkova, but @bogdandrutu requested reanaming it to "optional" in open-telemetry/opentelemetry-specification#2522 (comment):

I would actually consider to change "Opt-In" to "OPTIONAL", and say that instrumentation plugins must offers way to "opt-out" for any recommended attributes, and to "opt-in' for any available "optional" attributes. the required and conditional required cannot be disabled, this way we separate the 2 concerns about what is available and what is configurable for the user.

Personally, I would also still find "opt-in" clearer than "optional"

@Oberon00
Copy link
Member

Maybe it would be better to just change the markdown output, as everything else is a breaking change.

@arminru arminru added semconv/model Related to the data model or YAML format of the semantic convention generator semconv Related to the semantic convention generator. semconv/md Related specifically to the markdown output of the semantic convention generator labels Feb 20, 2023
@trask
Copy link
Member Author

trask commented Feb 20, 2023

Maybe it would be better to just change the markdown output, as everything else is a breaking change.

I split out the markdown rendering into a separate PR #137 in case there's a preference to move forward with that first and then we can discuss the breaking change on its own.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

I approved both PRs, depending on how you want to handle it in the spec and potentially code generators (I doubt anybody actually uses the requirement levels there though, except maybe for printing them out).

@trask
Copy link
Member Author

trask commented Feb 21, 2023

I approved both PRs, depending on how you want to handle it in the spec and potentially code generators (I doubt anybody actually uses the requirement levels there though, except maybe for printing them out).

thx! I'd prefer to merge this PR and trigger a new release of build-tools, then I will proceed with open-telemetry/opentelemetry-specification#3228

@trask
Copy link
Member Author

trask commented Feb 27, 2023

are there enough approvals on open-telemetry/opentelemetry-specification#3228 to merge this? thx!

@arminru
Copy link
Member

arminru commented Feb 28, 2023

@trask I believe so 🙂
If any strong disagreement comes up still we could revert build-tools before pulling it into the spec.

@arminru arminru merged commit 7bf090c into open-telemetry:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semconv/md Related specifically to the markdown output of the semantic convention generator semconv/model Related to the data model or YAML format of the semantic convention generator semconv Related to the semantic convention generator.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants