otel: create migration mechanism to semantic convention attribute names#45184
otel: create migration mechanism to semantic convention attribute names#45184fcfort wants to merge 10 commits into
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
LEft a couple of minor API comments.
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
Signed-off-by: Frank Fort <ffort@google.com>
|
/retest for failed TSAN timeout in //test/integration:quic_http_integration_test |
| // Configures usage of Semantic Conventions standard tag names. Specifying both options will | ||
| // emit both legacy and semantic convention tags. If neither are specified, only legacy Envoy | ||
| // tags are emitted. | ||
| message OtelSemconvStabilityOptIn { |
There was a problem hiding this comment.
BTW: Why is it called "OptIn" and not just "OtelSemconvStability"?
Can you please also add a link to where this is defined.
There was a problem hiding this comment.
Mostly to match the pattern from the OTEL_SEMCONV_STABILITY_OPT_IN environment variable from https://opentelemetry.io/docs/specs/semconv/http/. I added a comment with a link to this regard.
|
/assign @wbpcode |
Signed-off-by: Frank Fort <ffort@google.com>
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for this contribution. It's a good idea to use a enum to represent specific attributes.
But I guess the semantic convention should part of opentelmetry extension rather then the core of Envoy. But it's good idea to use an enum to represent well known attributes.
/wait
| /** | ||
| * Standardized tracing tag identifiers. | ||
| */ | ||
| enum class TagName { |
There was a problem hiding this comment.
| enum class TagName { | |
| enum class WellKnownTagName { |
| virtual void setTag(const Tag& tag, absl::string_view value) { | ||
| // Default implementation delegates to string-based name | ||
| setTag(tag.name(), value); | ||
| } |
There was a problem hiding this comment.
Rather than to use the Tag as input parameter, I prefer to use only the enum:
virtual void setTag(WellKnownTagName tag, absl::string_view value) PURE;
And rather than to update every span implementation to add using Tracing::Span::setTag;, we would prefer to add an implementation for every span implementation like:
void setTag(WellKnownTagName tag, absl::string_view value) {
setTag(Tracing::fromWellKnownTagName(tag), value);
}
The fromWellKnownTagName() could be a common helper that shared by every span implementation.
And the opentelemetry could override this new setTag to convert enum to semantic convention attribute name based on the configuration.
| class Tag { | ||
| public: | ||
| Tag(absl::string_view name, TagName id, absl::string_view sem_conv_name = "") | ||
| : name_(name), id_(id), sem_conv_name_(sem_conv_name.empty() ? name : sem_conv_name) {} | ||
|
|
||
| absl::string_view name() const { return name_; } | ||
| TagName id() const { return id_; } | ||
| absl::string_view semConvName() const { return sem_conv_name_; } | ||
|
|
||
| /** | ||
| * Implicit conversion to string_view enables transparent use at legacy call sites | ||
| * that expect string names. | ||
| */ | ||
| operator absl::string_view() const { return name_; } | ||
|
|
||
| bool operator==(const Tag& other) const { | ||
| return id_ == other.id_ && name_ == other.name_ && sem_conv_name_ == other.sem_conv_name_; | ||
| } | ||
|
|
||
| private: | ||
| const std::string name_; | ||
| const TagName id_; | ||
| const std::string sem_conv_name_; | ||
| }; |
There was a problem hiding this comment.
Per comment https://github.com/envoyproxy/envoy/pull/45184/changes#r3330240278, we may needn't this.
There was a problem hiding this comment.
If we change the setTag method from setTag(Tag) to setTag(WellKnownTagName) then we can't define an absl:string_view() operator on the enum and thus will be forced to change all existing setTag call-sites to take this new WellKnownTagName enum. This would be a breaking API change no? I don't see how we can do what you're suggesting.
There was a problem hiding this comment.
I may didn't make it clear. Note, the previous virtual void setTag(absl::string_view name, absl::string_view value) PURE; should still be kept along with the new virtual void setTag(WellKnonwTagname name, absl::string_view) PURE;.
And we can provide an implementation for new method like my comment at #45184 (comment). I don't think we need to change every call sites but only the call sites that will use the well tag known names?
There was a problem hiding this comment.
But that won't let us accomplish a migration. What we want is a way for users of the OTel tracer to be able to opt-in to using a different tag name for the same concept (i.e. the request method or response code). We need the existing call sites to call into the new method which we can accomplish using the technique I outlined in #30821. Having a new API that isn't backwards compatible will force us to change existing call sites in order to add new behavior.
There was a problem hiding this comment.
I don't think change existing call sites is unacceptable for these well known tags names.
If you do want to avoid that or want to try your best to avoid call sites change, you could update the class TracingTagValues's member type be the WellKnonwTagName (just like you current PR have updated it to new Tag type), then we needn't to change call sites. And the previous string values could be stored in constexpr string_view array and a helper could be used to convert the name to default string.
constexpr absl::string_view WellKnonwTagNameViews [] = {...}
absl::string_view wellKnonwTagnameToView(WellKnonwTagName name) {
return WellKnonwTagNameViews[static_cast<size_t>(name)];
}
For every span implementation (except Otel), you could add a default new setTag(WellKnonwTagName):
void setTag(WellKnonwTagName name, absl::string_view value) {
setTag(Tracing::wellKnonwTagnameToView(name), value);
}
From API's perspective, I don't think the sem_conv_name_ make sense. It should be part of implementation detail rather than part of abstract API. That's why I stick with we should only use the WellKnonwTagName only in the API.
Commit Message: otel: create migration mechanism to semantic convention attribute names
Additional Description:
This change implements the ability to bring Envoy’s OpenTelemetry tracer in alignment with the stable OpenTelemetry HTTP Semantic Conventions.
The architecture avoids adding additional string comparisons at core call sites by utilizing C++ overload resolution within the tracing driver APIs (
setTagoverload for typedTracing::Tag).Stability Opt-In Levels:
UNKNOWN: Defaults to legacy tags only (backward-compatible default behavior).LEGACY: Emits only the legacy Envoy attributes.LEGACY_AND_SEMCONV: Emits BOTH stable OTel and legacy Envoy attributes simultaneously.SEMCONV: Emits only the stable OpenTelemetry HTTP/Network semantic convention attributes.This was loosely inspired by the approach outlined in https://opentelemetry.io/docs/specs/semconv/http/ regarding
OTEL_SEMCONV_STABILITY_OPT_IN.Tag Migration Mapping:
The following table defines the exact mapping of span tags that will be migrated from the legacy Envoy naming conventions to the stable OpenTelemetry HTTP and Network semantic conventions under the respective opt-in levels:
http.methodhttp.request.methodhttp.status_codehttp.response.status_codehttp.urlurl.fullpeer.addressnetwork.peer.addressrequest_sizehttp.request.body.sizeresponse_sizehttp.response.body.sizeretry.counthttp.request.resend_countuser_agentuser_agent.originalNote:
http.protocolis deliberately NOT migrated tourl.schemeor any other semantic convention name because its semantics differ. It is unaffected by the opt-in levels and continues to emit only ashttp.protocol.Risk Level: Low (Adds standardized stability opt-in tags, defaults to backward-compatible legacy behavior).
Testing:
opentelemetry_tracer_impl_test.ccwith the newExportOTLPSpanWithLegacyOptInunit test to validate theLEGACYconfiguration behavior.//test/extensions/tracers/opentelemetry:opentelemetry_test) and affected core tracing and routing tests (//test/common/http:async_client_impl_testand//test/common/router:router_test).Docs Changes: None.
Release Notes: Added
otel_semconv_stability_opt_inconfiguration to support stable HTTP semantic convention attribute names in the OpenTelemetry tracer.Platform Specific Features: None.
Fixes #30821.
[Disclosed usage of generative AI: Yes, used to assist in code modifications.]