Adopt opentelemetry-semantic-conventions for OTel attribute names#3510
Adopt opentelemetry-semantic-conventions for OTel attribute names#3510ChihweiLHBird wants to merge 3 commits into
opentelemetry-semantic-conventions for OTel attribute names#3510Conversation
|
f8bb3c1 to
900797a
Compare
Thanks! Let me try that. Do we want to include migrations for them in this PR as well? |
Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
900797a to
e3e19ef
Compare
Up to you. |
|
Note that there are other experimental or deprecated semantic conventions that I left as string literals and didn't migrate to the constants in this PR. And it should be ready for review now. Let me know if anything else is needed. |
|
I probably should have started with this: looking at the diff now I'm not sure this is a net improvement. Avoiding typos is good, but there is also a downside here in that the constants make it harder to see the actual field names. That wouldn't be so bad if they were simply UPPERCASED versions of the field names but they also flatten periods into underscores... 🤔 |
|
@lann thanks for the thought! That's totally fair and I actually had a similar concern about it because this introduces another dependency. The upside is the omitted key is still the dotted conventions and benefits of safer compile-time typo checks. Another benefit is that the I am okay if the team decide to not accept this, and feel free to close the PR in that case. |
An example of this is |
calebschoepp
left a comment
There was a problem hiding this comment.
Did you make sure you found all possible attributes to replace?
Overall the advantages of getting warnings when things are deprecated seems worth it.
| @@ -1,5 +1,8 @@ | |||
| use anyhow::Result; | |||
| use http::Response; | |||
| use opentelemetry_semantic_conventions::attribute::{ | |||
There was a problem hiding this comment.
Maybe we could just import all of opentelemetry_semantic_conventions::attribute with an alias of otel_attribute or something like that.
There was a problem hiding this comment.
Good idea! Let me try that
There was a problem hiding this comment.
Getting an alias to work in a macro is more complex than I thought. It would require use ::opentelemetry_semantic_conventions::attribute as otel_attribute; in the macro block and also in the modules where the macro is used. Can I leave this as is?
There was a problem hiding this comment.
Using this pattern for all others seem to be fine and I pushed it here: 0b5917b
| @@ -397,12 +404,12 @@ impl p2::WasiHttpHooks for InstanceHttpHooks { | |||
| skip_all, | |||
| fields( | |||
| otel.kind = "client", | |||
There was a problem hiding this comment.
Is this one not a convention?
There was a problem hiding this comment.
Unfortunately, otel.kind is not an official convention and is a spin specific thing...
You can see all otel.* keys in the conventions on this page and it doesn't contain otel.kind.
There was a problem hiding this comment.
I would say another benefit of using the official convention crate is we can realize things like this earlier
There was a problem hiding this comment.
otel.* is from tracing-opentelemetry: https://docs.rs/tracing-opentelemetry/latest/tracing_opentelemetry/#special-fields
There was a problem hiding this comment.
If we're switching to constants for standard keys it would probably be wise for us to create constants for these too, e.g. OTEL_KIND, OTEL_NAME. (tracing-opentelemetry doesn't seem to define them)
Scope creepily: I wouldn't be sad to have macros for these. We use otel.name and otel.kind quite a bit so e.g.:
#[instrument(
otel_kind!("client"),
// Lots of this pattern in crates/factor-outbound-redis/src/host.rs
otel_name!("GET {key}"),
)]expanding to
#[instrument(
{$crate::something_goes_here::OTEL_KIND} = "client",
// Lots of this pattern in crates/factor-outbound-redis/src/host.rs
{$crate::something_goes_here::OTEL_NAME} = format!("GET {path}"),
)]Note: if you haven't seen it, $crate is a magic macro variable: https://doc.rust-lang.org/reference/macros-by-example.html#r-macro.decl.meta.dollar-crate
There was a problem hiding this comment.
Scope creepily: I wouldn't be sad to have macros for the otel.* fields. We use otel.name and otel.kind quite a bit
This looks like something for tracing-opentelemetry to add.
|
@calebschoepp thanks for the review!
There are some experimental keys and some deprecated key I didn't replace with constants. The experimental keys require an additional feature of the crate, and the deprecate keys requir |
|
I was on the fence so @calebschoepp can be the tie-breaker; I delegate my review to him. 🙂 |
Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
7f046d0 to
2424006
Compare
Updated the usage of OpenTelemetry semantic conventions in the outbound HTTP crate to utilize the new aliasing for attribute names. Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
Description
Adopt
opentelemetry-semantic-conventions0.28 for stable runtime span attribute names on outbound HTTP (factor-outbound-http) and the HTTP trigger (trigger-http), replacing string-literal field keys with crate constants where available.Bump workspace
tracingfrom 0.1.41 to 0.1.44 so#[instrument(fields(...))]accepts braced semconv constants (oldertracing-attributesonly parsed dotted field names there).otel.kind,otel.name, and incubatinghttp.response.body.sizestay literals (with a short comment on body size). Spin-specific attributes (e.g.error.blame) are unchanged.