[otelfiber] Fixed incompatibility issues with new otel v1.12.0/v0.35.0 release

[eko]

Context

OpenTelemetry has released v1.12.0/v0.35.0 which have some backward compatibility issues with previous release on method namings.

Release note says:

Metric instruments to go.opentelemetry.io/otel/metric/instrument. These instruments are use as replacements of the depreacted go.opentelemetry.io/otel/metric/instrument/{asyncfloat64,asyncint64,syncfloat64,syncint64} packages.(open-telemetry/opentelemetry-go#3575, open-telemetry/opentelemetry-go#3586)
Float64ObservableCounter replaces the asyncfloat64.Counter
Float64ObservableUpDownCounter replaces the asyncfloat64.UpDownCounter
Float64ObservableGauge replaces the asyncfloat64.Gauge
Int64ObservableCounter replaces the asyncint64.Counter
Int64ObservableUpDownCounter replaces the asyncint64.UpDownCounter
Int64ObservableGauge replaces the asyncint64.Gauge
Float64Counter replaces the syncfloat64.Counter
Float64UpDownCounter replaces the syncfloat64.UpDownCounter
Float64Histogram replaces the syncfloat64.Histogram
Int64Counter replaces the syncint64.Counter
Int64UpDownCounter replaces the syncint64.UpDownCounter
Int64Histogram replaces the syncint64.Histogram

Changes

This pull request upgrades the following packages to new minor release:

• go.opentelemetry.io/otel/metric (v0.34.0 -> v0.35.0)
• go.opentelemetry.io/otel/sdk/metric (v0.34.0 -> v0.35.0)

This pull request also fixes the incompatibility issues.

Thank you for your great community work!

[gaby]

@onematchfox Can you help reviewing this PR?

[onematchfox]

LGTM so far. I would, however, recommend upgrading the semconv references (here and here) to align with the latest version, 1.16, in this release as well. Reading the release notes, I suspect this will entail additional method call changes. E.g. From using HTTPAttributesFromHTTPStatusCode to ClientResponse. @eko, are you good with making these additional changes?

[onematchfox]

Replaces #431 and #432.

[eko]

Thank you, I will take a look to do these additional changes tomorrow :)
I will keep you updated

[eko]

@onematchfox I just upgraded to the go.opentelemetry.io/otel/semconv/v1.17.0 package dependency (latest version).

I changed the method calls but there is also this additional change:

• Key HTTPServerNameKey is deleted since a commit in v1.13.0 so I've removed it.

Please let me know if you want me to use another key but I haven't found a good replacement for this key.

[gaby]

@eko I havent found one either. I also noticed that some big projects are using v1.10 for semconv instead of the latest one.

[onematchfox]

I changed the method calls but there is also this additional change:

• Key HTTPServerNameKey is deleted since a commit in v1.13.0 so I've removed it.

Thanks. I had missed this when scanning the release notes, but this also doesn't come as any real surprise to me. #409 already contained the first step in moving away from using this attribute. Unfortunately, the OTEL semantic conventions are a moving target at this point. However, there is a push to achieve stability for HTTP conventions underway, so hopefully, things should stabilise soon.

As for how to handle this change, my vote would go for dropping the attribute (as you have done) while also:

• Marking ServerName and WithServerName as deprecated since ServerName is no longer used.
• Explicitly calling out in the release notes that http.server_name will no longer be output in metrics and traces in line with the latest HTTP semantic conventions. Users should adjust consumers of metrics/traces to make use of net.host.name instead.

However, I would completely understand anyone who opposes this approach as well. @gaby happy to let you make the call here.

[ReneWerner87]

@onematchfox @gaby says if the pull request is mergable and preferably does not contain any breaking changes

[eko]

I can still use the latest version that supports the attribute for now and wait end of discussions and update package version and deprecate the attribute when attributes discussions will be stabilised if it is confirmed that they want to remove it.

[onematchfox]

I can still use the latest version that supports the attribute for now

That would be 1.12 - which the module referenced prior to this PR.

when attributes discussions will be stabilised if it is confirmed that they want to remove it

I don't believe http.server_name is coming back - it was dropped from the specification in April last year. There's a bunch of discussion in that PR related to this attribute, but in essence, the decision was:

Remove http.server_name - if it's available it's populated in net.host.name

[onematchfox]

@onematchfox @gaby says if the pull request is mergable

I think you may have missed a word or two after "@gaby says"... ?

and preferably does not contain any breaking changes

So, that's the tricky thing... Marking ServerName and WithServerName as deprecated and calling the method out as a no-op avoids a breaking change to the interface of this module. However, removing the attribute from any traces/metrics could be seen as one for any consumers of the resulting traces/metrics. IMO, given that this will need to happen at some point, I say we "bite the bullet" and do so now while explicitly calling out the alternative/upgrade path in the release notes. However, I also acknowledge my acceptance of "friction" is going to be different to others who may completely disagree with me here. If folks would prefer the path of least friction then, let's deprecate ServerName and WithServerName in this PR while leaving the module referencing semconv/v1.12.0 for the time being. That at least gives people a chance to migrate any consumers of the traces/metrics and highlight any issues they might have in doing so.

[onematchfox]

Looking at the implementation in this module, net.host.name is not a drop-in replacement for http.server_name at this point. I have also been perusing open-telemetry/opentelemetry-go-contrib#3182 for inspiration on how to approach the upgrade to the semconv packages - which has led me to question if the current implementation in this library is correct or not. As such, I have concluded that the semconv upgrade would be best handled in a separate PR. That way, we don't hold up this change and can ensure sufficient thought/discussion is given to the semconv changes. @eko would you mind reverting out the changes I asked for? Sorry for the back and forth!

[eko]

@onematchfox No problem, I just reverted the changes concerning semconv. Thank you for your research and taking time to discuss this!

[onematchfox]

@gaby @ReneWerner87 I think these changes are good to go.

P.S. I'll try to find some time in the coming week or two to do some more digging on the semconv changes and open up a PR once I have.

[ReneWerner87]

@WLun001 @lucasoares can someone confirm the change with, am not too deep in the middleware

[gaby]

I'm about to look at the changes. I do agree with @onematchfox. I have been looking at other projects and everyone is doing semconv differently.

[gaby]

I have been looking at the implementation by go-redis which is up to date. They use semconv 1.10, see here

[gaby]

Looking at darp, they also use 1.10, see here

They also have a fasthttp HTTP metrics tracer here

[gaby]

@eko They just released v0.36.0 with more changes! 😂

[ReneWerner87]

@gaby is the pull request mergeable ? for these versions ? further migration can be done afterwards, if this works currently(without breaking changes)

[gaby]

@ReneWerner87 After the last commit reverting to 0.12.0, it should be good once the CI passes.

[ReneWerner87]

@eko can you update and resolve the conflicts

[eko]

@ReneWerner87 @gaby Thank you, I just resolved conflicts and updated to v0.36.0 (latest release) which breaks anything.