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

On branch edburns-msft-em-5989-review-2-0 #232

Merged

Conversation

edburns
Copy link
Contributor

@edburns edburns commented Aug 19, 2024

modified: spec/src/main/asciidoc/metrics.adoc

Two trivial changes.

  • Clarify that conditions are per-attribute. Might not be obvious to someone not deeply familiar with SemConv.

  • SemConv uses Opt-In. I corrected the case.

modified:   spec/src/main/asciidoc/metrics.adoc

Two trivial changes.

- Clarify that conditions are per-attribute. Might not be obvious to someone not deeply familiar with SemConv.

- SemConv uses `Opt-In`. I corrected the case.

Signed-off-by: Ed Burns <[email protected]>
@donbourne
Copy link
Member

@edburns , your changes in this PR look good to me and IMO would be fine to be in the next release. I'll wait for our meetings to resume in September to discuss with others on the MP Telemetry call.

Regarding your comment in the vote thread...

I have a question. This question pertains in both SemConv v1.27.0 and v1.26.0, so it is relevant to MP 2.0:

Consider this text from: https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/#conditionally-required

When a Conditionally Required attribute’s condition is not satisfied, and there is no requirement to populate the attribute, semantic conventions MAY provide special instructions on how to handle it. If no instructions are given and if instrumentation can populate the attribute, instrumentation SHOULD use the Opt-In requirement level on the attribute.

I wonder if MP telemetry should say something about what MP telemetry implementations should do in this case? Not required but just curious. Maybe forward this on to Don Bourne?

I can see that our guidance in the MP Telemetry spec doesn't cover all cases. Our spec says "All attributes that are listed as conditionally required and stable in the OpenTelemetry Semantic Conventions MUST be included when the per-attribute condition described in the OpenTelemetry Semantic Conventions is satisfied." -- which I realize doesn't address specifically what to do when the per-attribute condition is not satisfied. Our intent is that people follow the guidance from the semantic convention for conditional attributes (not just the guidance when the condition is satisfied), so that could be better worded. If you want to open an issue for that or add text for that to this PR, we can discuss a good resolution.

Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @edburns

@Emily-Jiang
Copy link
Member

@donbourne will provide an additional PR to address your concerns in the voting thread @edburns

@Emily-Jiang
Copy link
Member

related to #235

@Emily-Jiang Emily-Jiang merged commit fca8b82 into eclipse:main Sep 9, 2024
1 of 3 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.

3 participants