otelhttp: client metrics #4707

Sovietaced · 2023-12-17T07:25:41Z

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 · 2023-12-17T07:29:53Z

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

instrumentation/net/http/otelhttp/common.go

instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go

instrumentation/net/http/otelhttp/transport.go

instrumentation/net/http/otelhttp/transport_test.go

pellared · 2023-12-28T16:43:29Z

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 · 2023-12-28T16:45:47Z

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.

instrumentation/net/http/otelhttp/test/transport_test.go

instrumentation/net/http/otelhttp/common.go

instrumentation/net/http/otelhttp/transport.go

instrumentation/net/http/otelhttp/common.go

pellared · 2023-12-28T17:22:27Z

Please add a changelog entry.

Sovietaced · 2023-12-28T22:40:45Z

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 :)

instrumentation/net/http/otelhttp/common.go

#4707 (comment)

instrumentation/net/http/otelhttp/common.go

instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go

Sovietaced · 2024-01-07T06:02:02Z

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

instrumentation/google.golang.org/grpc/otelgrpc/example/api/hello-service.pb.go

instrumentation/net/http/otelhttp/transport.go

Sovietaced · 2024-02-01T08:00:40Z

@pellared should be good to go

pellared

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

instrumentation/net/http/otelhttp/transport_test.go

instrumentation/net/http/otelhttp/transport.go

Co-authored-by: Robert Pająk <[email protected]>

gaiaz-iusipov · 2024-02-02T13:13:52Z

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 · 2024-02-02T22:44:33Z

@Sovietaced Thank you for your contribution

Sovietaced · 2024-02-03T20:24:25Z

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 · 2024-02-09T12:05:54Z

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 · 2024-02-09T12:12:16Z

instrumentation/net/http/otelhttp/test/handler_test.go

@@ -50,8 +50,8 @@ func assertScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribut
 	require.Len(t, sm.Metrics, 3)

 	want := metricdata.Metrics{
-		Name:        "http.server.request_content_length",
-		Description: "Measures the size of HTTP request content length (uncompressed)",
+		Name:        "http.server.request.size",


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

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

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.

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.

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).

Sovietaced changed the title ~~Initial stab at http client metrics~~ otelhttp: client metrics Dec 17, 2023

Sovietaced mentioned this pull request Dec 17, 2023

[otelhttp] transport metrics #3769

Open

Sovietaced marked this pull request as ready for review December 17, 2023 21:22

Sovietaced requested review from Aneurysm9 and dmathieu as code owners December 17, 2023 21:22

Sovietaced requested a review from a team December 17, 2023 21:22

Sovietaced force-pushed the otelhttp-transport-metrics branch from 422e763 to 4351562 Compare December 17, 2023 22:36

ash2k suggested changes Dec 18, 2023

View reviewed changes

Sovietaced requested review from hanyuancheung and dashpole as code owners December 18, 2023 07:50

Sovietaced force-pushed the otelhttp-transport-metrics branch from 079f76a to dd0a2fd Compare December 18, 2023 08:17

MadVikingGod reviewed Dec 18, 2023

View reviewed changes

instrumentation/net/http/otelhttp/transport_test.go Outdated Show resolved Hide resolved

pellared reviewed Dec 28, 2023

View reviewed changes

Sovietaced mentioned this pull request Dec 28, 2023

Add documentation to otelhttp package #4774

Open

Sovietaced force-pushed the otelhttp-transport-metrics branch from 22539b3 to 2b2c02e Compare December 29, 2023 05:33

pellared reviewed Dec 29, 2023

View reviewed changes

instrumentation/net/http/otelhttp/common.go Outdated Show resolved Hide resolved

pellared previously approved these changes Dec 29, 2023

View reviewed changes

SodaDev reviewed Dec 29, 2023

View reviewed changes

instrumentation/net/http/otelhttp/common.go Outdated Show resolved Hide resolved

instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go Show resolved Hide resolved

Sovietaced force-pushed the otelhttp-transport-metrics branch from bdf65dd to 50aa699 Compare December 29, 2023 23:29

pellared approved these changes Dec 30, 2023

View reviewed changes

dmathieu reviewed Jan 8, 2024

View reviewed changes

instrumentation/google.golang.org/grpc/otelgrpc/example/api/hello-service.pb.go Outdated Show resolved Hide resolved

Add support for http client metrics

f54fc6c

Sovietaced force-pushed the otelhttp-transport-metrics branch from 50aa699 to f54fc6c Compare January 9, 2024 00:34

MadVikingGod reviewed Jan 29, 2024

View reviewed changes

instrumentation/net/http/otelhttp/transport.go Outdated Show resolved Hide resolved

MadVikingGod reviewed Jan 29, 2024

View reviewed changes

instrumentation/net/http/otelhttp/transport.go Show resolved Hide resolved

Sovietaced added 2 commits January 29, 2024 12:32

Add clarifying comments

f41446d

Add back removed constants and make note about deprecation

70c1791

Sovietaced force-pushed the otelhttp-transport-metrics branch from adaeed4 to 70c1791 Compare January 29, 2024 20:57

MadVikingGod approved these changes Jan 31, 2024

View reviewed changes

merge conflicts

0c65b90

pellared reviewed Feb 1, 2024

View reviewed changes

instrumentation/net/http/otelhttp/transport_test.go Outdated Show resolved Hide resolved

instrumentation/net/http/otelhttp/transport_test.go Outdated Show resolved Hide resolved

instrumentation/net/http/otelhttp/transport.go Show resolved Hide resolved

MadVikingGod added the instrumentation: otelhttp label Feb 1, 2024

Sovietaced and others added 3 commits February 1, 2024 23:13

Update instrumentation/net/http/otelhttp/transport_test.go

25dc08a

Co-authored-by: Robert Pająk <[email protected]>

Update instrumentation/net/http/otelhttp/transport_test.go

1251f7e

Co-authored-by: Robert Pająk <[email protected]>

Add comment abour using atomic.Bool instead of sync.Once

3886144

Harnoor7 mentioned this pull request Feb 2, 2024

Initial implementation krakend/krakend-otel#1

Merged

pellared approved these changes Feb 2, 2024

View reviewed changes

Merge branch 'main' into otelhttp-transport-metrics

cac99f8

pellared merged commit b76d81c into open-telemetry:main Feb 2, 2024
22 checks passed

Sovietaced deleted the otelhttp-transport-metrics branch February 3, 2024 19:39

MrAlias mentioned this pull request Feb 6, 2024

Release v1.23.0/v0.48.0/v0.17.0/v0.3.0 #4893

Merged

MrAlias added this to the v0.48.0 milestone Feb 6, 2024

MrAlias mentioned this pull request Feb 7, 2024

Data race in v0.48.0 #4895

Closed

StarpTech reviewed Feb 9, 2024

View reviewed changes

dashpole mentioned this pull request Feb 9, 2024

Update changelog to note changes to http request/response size metric names #4898

Merged

This was referenced Mar 1, 2024

Calling otelgrpc.UnaryClientInterceptor repeatedly creates multiple Int64Histogram resulting in a memory leak #4226

Closed

Memory leak caused by otelhttp when no Meter Provider is configured. #5190

Closed

StarpTech mentioned this pull request Mar 27, 2024

Move otelhttp/internal/semconvutil into otelhttp #4580

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

otelhttp: client metrics #4707

otelhttp: client metrics #4707

Sovietaced commented Dec 17, 2023 •

edited

Loading

codecov bot commented Dec 17, 2023 •

edited

Loading

pellared commented Dec 28, 2023

pellared commented Dec 28, 2023

pellared commented Dec 28, 2023

Sovietaced commented Dec 28, 2023

Sovietaced commented Jan 7, 2024

Sovietaced commented Feb 1, 2024

pellared left a comment

gaiaz-iusipov commented Feb 2, 2024

pellared commented Feb 2, 2024

Sovietaced commented Feb 3, 2024

StarpTech commented Feb 9, 2024

StarpTech Feb 9, 2024 •

edited

Loading

dashpole Feb 9, 2024

StarpTech Feb 9, 2024 •

edited

Loading

dashpole Feb 9, 2024

StarpTech Feb 9, 2024

otelhttp: client metrics #4707

otelhttp: client metrics #4707

Conversation

Sovietaced commented Dec 17, 2023 • edited Loading

codecov bot commented Dec 17, 2023 • edited Loading

Codecov Report

pellared commented Dec 28, 2023

pellared commented Dec 28, 2023

pellared commented Dec 28, 2023

Sovietaced commented Dec 28, 2023

Sovietaced commented Jan 7, 2024

Sovietaced commented Feb 1, 2024

pellared left a comment

Choose a reason for hiding this comment

gaiaz-iusipov commented Feb 2, 2024

pellared commented Feb 2, 2024

Sovietaced commented Feb 3, 2024

StarpTech commented Feb 9, 2024

StarpTech Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

dashpole Feb 9, 2024

Choose a reason for hiding this comment

StarpTech Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

dashpole Feb 9, 2024

Choose a reason for hiding this comment

StarpTech Feb 9, 2024

Choose a reason for hiding this comment

Sovietaced commented Dec 17, 2023 •

edited

Loading

codecov bot commented Dec 17, 2023 •

edited

Loading

StarpTech Feb 9, 2024 •

edited

Loading

StarpTech Feb 9, 2024 •

edited

Loading