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

Address the release review feedback from Ed Burns #235

Open
Emily-Jiang opened this issue Sep 9, 2024 · 2 comments
Open

Address the release review feedback from Ed Burns #235

Emily-Jiang opened this issue Sep 9, 2024 · 2 comments
Assignees

Comments

@Emily-Jiang
Copy link
Member

Description

Ed wrote in the spec release review voting thread with the following info.

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?
@donbourne
Copy link
Member

From discussion on today's MP Telemetry call:

In the MP Telemetry 2.0 spec, we list the following conditionally required metric attributes on the http.server.request.duration metric:

  • error.type

    • OTel sem conv says: Conditionally Required If request has ended with an error.
  • http.response.status_code

    • OTel sem conv says: Conditionally Required If and only if one was received/sent.
  • http.route

    • OTel sem conv says: Conditionally Required If and only if it’s available

In the 3 attributes listed above, it's fairly clear that if the info isn't available or doesn't apply then implementations shouldn't fill it in. We discussed that we didn't want the MP Telemetry spec to say that impls must not provide the attribute in those situations since there are cases where including the attribute with an empty string as the value is more appropriate. One such case is for impls providing a /metrics endpoint, which will likely use the Prometheus client, and the Prometheus client requires all instances of a metric with the same name to use the same set of attribute names.

  • network.protocol.name
    • OTel sem conv says: If not http and network.protocol.version is set.

For the attribute above, we discussed that we thought MP implementations would likely always use http as the value, since none of the other values listed seemed appropriate. We also discussed that we should open an issue with OpenTelemetry semantic convention to get clarity on what "If not http and network.protocol.version is set" means.

In summary, while the MP Telemetry spec doesn't say what to do when conditions for conditionally required attributes are not satisfied, the conditions themselves make it reasonably clear what implementers should do already.

@donbourne
Copy link
Member

@edburns , please take a look at the comment above -- does that adequately address the concern you had earlier?

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

No branches or pull requests

2 participants