open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.
https://opentelemetry.io/
Apache License 2.0
1.13k stars 536 forks source link

Upgrade v1.24.0 to v1.26.0 semconv for otelhttp #5902

Closed MrAlias closed 1 month ago

MrAlias commented 1 month ago

An audit of the new semconv for any additions missing will be needed.

VinozzZ commented 1 month ago

Hi @MrAlias , I would like to work on this issue.

FYI, based on this message in CNCF's slack , it's called out by the Semconv Maintainers that adoption for HTTP should wait until final migration to stabilized semconv is ready.

MrAlias commented 1 month ago

Hi @MrAlias , I would like to work on this issue.

FYI, based on this message in CNCF's slack , it's called out by the Semconv Maintainers that adoption for HTTP should wait until final migration to stabilized semconv is ready.

The note you linked to is referring to database semantic conventions:

Quite a few DB related breaking changes are in this release. As with HTTP, please hold on adopting these until final migration to stabilized semconv is ready.

The HTTP semantic conventions are already stable: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-spans.md

Though an audit of any additions between v1.24.0 and v1.26.0 needs to be done. These are allowed stable changes.

VinozzZ commented 1 month ago

Thank you for the clairification!

It looks like we have a few new attributes that currently are all under experimental status Add url.template to HTTP client attributes Add experimental HTTP attributes to HTTP spans. The list of attributes are

For both client and server side:

For only client side:

MrAlias commented 1 month ago

Gotcha :+1: . We'll want to not add anything experimental at this point.

Sounds like the upgrade without those additions should just be a version bump then.

VinozzZ commented 1 month ago

I assume we will do this after we finish adding support for v1.24.0. Is that correct?

MrAlias commented 1 month ago

I assume we will do this after we finish adding support for v1.24.0. Is that correct?

Possibly. Adding experimental semantic conventions is what got us into this dual export situation.

We will likely not add them at all until stable. Or add them behind a flag.

VinozzZ commented 1 month ago

That makes sense.

I'm trying to figure out where we define the semconv version is defined for otelhttp. Is this the right place? https://github.com/open-telemetry/opentelemetry-go-contrib/blob/dedcf91a55a36a5a8589c56f2e43c188eb42f4f2/instrumentation/net/http/otelhttp/version.go#L7

MrAlias commented 1 month ago

Here: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/dedcf91a55a36a5a8589c56f2e43c188eb42f4f2/instrumentation/net/http/otelhttp/internal/semconv/v1.24.0.go#L11

That file likely needs to be renamed as well. Please rename to something that doesn't include the version in the name.

MadVikingGod commented 1 month ago

5923 fixed this.