eclipse / microprofile-telemetry

microprofile telemetry
Apache License 2.0
19 stars 18 forks source link

Address the release review feedback from Ed Burns #235

Open Emily-Jiang opened 2 months ago

Emily-Jiang commented 2 months ago

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 commented 2 months ago

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:

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.

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 commented 2 months ago

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