otelhttp: client metrics

[Sovietaced]

This pull request attempts to follow up on the work that @RangelReale did in #3769 and close out the work. This pull request adds support for http client metrics that mostly adhere to the v1.20 semantic conventions. In many ways this code attempts to mirror the http server metric code in implementation.

As such, it reuses the HTTP request body wrapper logic and introduces HTTP response body wrapper logic with callbacks to record metrics once the response body has been read.

Open Questions:

• As written these client metrics follow the convention of the server metrics, but don't actually follow the convention of the OTEL spec as noted in Metrics are non conformant to semantic conventions #4501. I guess this migration can be done later?
• I don't believe the code will report HTTP response metrics unless the response body is read, which is the responsibility of the client. Are we ok with this behavior? I tried to follow the the pattern where reading bodies is done with wrapping to minimize allocations.
• It is not clear to me how the body wrappers work with long lived connections?

Fixes #3134
Fixes #1336

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5047be2) 79.7% compared to head (cac99f8) 80.3%.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #4707     +/-   ##
=======================================
+ Coverage   79.7%   80.3%   +0.5%     
=======================================
  Files        151     151             
  Lines       9959   10209    +250     
=======================================
+ Hits        7946    8201    +255     
+ Misses      1859    1854      -5     
  Partials     154     154             

Files	Coverage Δ
...stful/otelrestful/internal/semconvutil/httpconv.go	95.6% <100.0%> (+0.3%)	⬆️
...gonic/gin/otelgin/internal/semconvutil/httpconv.go	95.6% <100.0%> (+0.3%)	⬆️
...rilla/mux/otelmux/internal/semconvutil/httpconv.go	95.6% <100.0%> (+0.3%)	⬆️
...ack/echo/otelecho/internal/semconvutil/httpconv.go	95.6% <100.0%> (+0.3%)	⬆️
...on.v1/otelmacaron/internal/semconvutil/httpconv.go	95.6% <100.0%> (+0.3%)	⬆️
...ace/otelhttptrace/internal/semconvutil/httpconv.go	95.6% <100.0%> (+0.3%)	⬆️
instrumentation/net/http/otelhttp/common.go	100.0% <ø> (ø)
instrumentation/net/http/otelhttp/handler.go	87.3% <100.0%> (ø)
...net/http/otelhttp/internal/semconvutil/httpconv.go	95.6% <100.0%> (+0.3%)	⬆️
instrumentation/net/http/otelhttp/transport.go	96.9% <100.0%> (+2.1%)	⬆️

... and 2 files with indirect coverage changes

[pellared]

I don't believe the code will report HTTP response metrics unless the response body is read, which is the responsibility of the client. Are we ok with this behavior? I tried to follow the the pattern where reading bodies is done with wrapping to minimize allocations.

We are fine. From https://pkg.go.dev/net/http:

The caller must close the response body when finished with it:
...
Caller should close resp.Body when done reading from it.

Also we already have this requirement for trace instrumentation.

Side note: probably we should call it out in package documentation.

[pellared]

As written these client metrics follow the convention of the server metrics, but don't actually follow the convention of the OTEL spec as noted in #4501. I guess this migration can be done later?

We want to first be be compliant with the v1.20 semantic conventions. Next semantic convention versions have many breaking changes and we want to help a little with the migration.

[pellared]

Please add a changelog entry.

[Sovietaced]

Please add a changelog entry.

Will do, I didn't add one since you seemed a bit picky about the changelog entries on my last PR :)

#4707 (comment)

[Sovietaced]

@dmathieu @Aneurysm9 @dashpole @hanyuancheung Please take a look when you have a chance. TY

[Sovietaced]

@pellared should be good to go

[pellared]

Little comments that I would like to have addressed. Besides looks good 👍

[gaiaz-iusipov]

It would be nice to have something similar to otelhttp.WithRouteTag for the transport, thus allowing the separation of different requests in metrics. For example: otelhttp.WithRequestTag.

[pellared]

@Sovietaced Thank you for your contribution

[Sovietaced]

It would be nice to have something similar to otelhttp.WithRouteTag for the transport, thus allowing the separation of different requests in metrics. For example: otelhttp.WithRequestTag.

Maybe create a separate issue for this?

[StarpTech]

Hi, it would be great if new features like metrics are built behind a feature flag. For example, we use the middleware for everything expect metrics. As a workaround, we pass a Noop meter.

[StarpTech]

@pellared This looks to me like a breaking change. Why wasn't it communicated as such?

[dashpole]

@sharptech sorry about that. I've opened #4898 to add it to the changelog.

[StarpTech]

Hi @dashpole wouldn't it be better to rollback so the deprecation statements remains true? I saw that the old name isn't used at all in the code. I also think breaking stuff with a feature release isn't the right approach.

[dashpole]

The deprecation statements refer to these constants: https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4707/files#diff-be9e2412d1919396470e196d2be4bddf6997059892167fd8381a9cfa478933faR34

I also think breaking stuff with a feature release isn't the right approach.

The otelhttp module is not stable, and is still released under a v0.* version, so breaking changes are expected. You can see the versioning policy here: https://github.com/open-telemetry/opentelemetry-go/blob/main/VERSIONING.md. The telemetry in otelhttp follows the semantic conventions for http metrics: https://opentelemetry.io/docs/specs/semconv/http/http-metrics/, so breaking changes to conventions require us to make breaking changes here.

[StarpTech]

I understand, but users like me running this in production. We need to deprecate fields before we can upgrade. If I would want to use the new metrics I couldn't. I'm saying the feature and the breaking change could have been shipped in two PR (releases).