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 #3228

Merged
merged 4 commits into from
Mar 1, 2023

Conversation

trask
Copy link
Member

@trask trask commented Feb 17, 2023

Fixes #3227

Changes

Renames "Optional" attribute requirement level to "Opt-In", to better reflect it's meaning.

For non-trivial changes, follow the change proposal process and link to the related issue(s) and/or OTEP(s), update the CHANGELOG.md, and also be sure to update spec-compliance-matrix.md if necessary.

semantic_conventions/trace/http.yaml Outdated Show resolved Hide resolved
@Oberon00
Copy link
Member

Copying my comment from open-telemetry/build-tools#135 (comment):

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 #2522 by @lmolkova, but @bogdandrutu requested reanaming it to "optional" in #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"

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.

@bertysentry
Copy link
Contributor

I concur with @Oberon00: When specifying semantic conventions, we need to have different nuances for specifying the requirement (or not) of an attribute:

  • Required (e.g. a unique ID)
  • Recommended (e.g. a name, human-readable)
  • Optional (e.g. extra information about the observed resource, not necessary to identify it, but helpful)

For Recommended and Optional, we could have a separate qualifier (e.g. Opt-in and Opt-out) which indicates whether this attribute can be enabled or disabled manually by the user (e.g. an IP address, a local file path, etc. for security reasons).

@Oberon00
Copy link
Member

Oberon00 commented Feb 21, 2023

I concur with @Oberon00:

I wasn't suggesting to add an additional opt-out level. I think that's too fine grained and you should decide between "recommended" and "opt-in". I actually concur with this PR which stays with our current number of levels and just renames the "optional" level to better match its definition.

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Feb 21, 2023
@trask trask marked this pull request as ready for review February 23, 2023 16:21
@trask trask requested review from a team February 23, 2023 16:21
@reyang reyang merged commit ec84e5d into open-telemetry:main Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Optional attribute requirement level to Opt-In