[otelhttp] transport metrics #3769

RangelReale · 2023-05-03T13:00:30Z

Add HTTP client metrics following the semantic conventions.

As users of Transport are not expected to have a handler where they can customize the attributes using a Labeler, callback function options were added to add attributes from the request and the reponse objects.

Fixes #3134
Fixes #1336

codecov · 2023-05-04T01:54:05Z

Codecov Report

Merging #3769 (45ceb1d) into main (7d9626d) will increase coverage by 0.0%.
Report is 1 commits behind head on main.
The diff coverage is 94.4%.

❗ Current head 45ceb1d differs from pull request most recent head e28520c. Consider uploading reports for the commit e28520c to get more accurate results

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #3769   +/-   ##
=====================================
  Coverage   79.4%   79.4%           
=====================================
  Files        166     166           
  Lines      10360   10401   +41     
=====================================
+ Hits        8230    8266   +36     
- Misses      1996    1999    +3     
- Partials     134     136    +2

Files Changed	Coverage Δ
instrumentation/net/http/otelhttp/common.go	`100.0% <ø> (ø)`
instrumentation/net/http/otelhttp/transport.go	`94.1% <93.4%> (-0.7%)`	⬇️
instrumentation/net/http/otelhttp/config.go	`84.6% <100.0%> (+1.7%)`	⬆️
instrumentation/net/http/otelhttp/handler.go	`86.9% <100.0%> (ø)`
instrumentation/net/http/otelhttp/wrap.go	`92.5% <100.0%> (+7.4%)`	⬆️

... and 3 files with indirect coverage changes

dmathieu · 2023-05-04T08:24:42Z

This would need a changelog, and tests.

RangelReale · 2023-05-04T12:59:22Z

This would need a changelog, and tests.

Add tests and changelog.

RangelReale · 2023-05-04T14:16:14Z

Hmm CI shows a data race message that I can't reproduce locally.

pellared · 2023-05-08T11:09:42Z

Hmm CI shows a data race message that I can't reproduce locally.

For reference: https://github.com/open-telemetry/opentelemetry-go-contrib/actions/runs/4883405160/jobs/8714784942

Have you tried running multiple times? E.g. by calling go test -race -count=1000 ?

EDIT: ~~At first glance it looks not related to your changes. If I am correct, then it would be better to create a separate issue for it.~~

EDIT 2: The data race is in your code. #3769 (comment). PS. I managed to get the data race on my machine (but the probability to hit it is extremely low. I had to run go test -race -count=10000 multiple times in instrumentation/net/http/otelhttp/test directory).

instrumentation/net/http/otelhttp/transport.go

forestsword

Thanks so much for this. I was looking for this feature. Until this gets merged I decided to follow your lead and did something similar in what I'm writing. I'm not a maintainer but my review is some things I found.

forestsword · 2023-05-09T15:33:26Z

instrumentation/net/http/otelhttp/common.go

@@ -37,6 +37,14 @@ const (
 	ServerLatency         = "http.server.duration"                // Incoming end to end duration, microseconds
 )

+// Client HTTP metrics.
+const (
+	ClientRequestCount          = "http.client.request_count"           // Outgoing request count total


This is no longer defined in the semantic convention. And doesn't look to be actually used in the code.

Yes it is not used, we are supposed to get this information from the count of duration. I left it there becase the one for server was also there.

Looks like the server one is still there, so I'm keeping this one also.

Please adopt the v1.17 semantic convection for HTTP Client.

See: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.17.0/specification/metrics/semantic_conventions/http-metrics.md#http-client

instrumentation/net/http/otelhttp/transport.go

pellared · 2023-07-27T15:47:19Z

@RangelReale are you still willing to work on this?

RangelReale · 2023-07-27T15:55:16Z

yes, I''m available either to follow up or if someone wants to takeover also fine.

pellared · 2023-07-27T16:19:15Z

yes, I''m available either to follow up or if someone wants to takeover also fine.

It is fine if you follow up. I will do my best to take look in the beginning of next week.

instrumentation/net/http/otelhttp/transport.go

instrumentation/net/http/otelhttp/wrap.go

pellared

There are build failures.

Moreover, this PR MUST NOT add anything more than defined in https://github.com/open-telemetry/opentelemetry-specification/blob/v1.17.0/specification/metrics/semantic_conventions/http-metrics.md#http-client

pellared · 2023-08-10T08:22:37Z

@Aneurysm9, can you please review this PR (as module codeowner)?

pellared

I did my best to carefully review this PR. Nice work. A left a few comments that needs to be addressed. Some should lead to simplification of the PR.

instrumentation/net/http/otelhttp/common.go

pellared · 2023-08-10T08:39:03Z

instrumentation/net/http/otelhttp/config.go

+// WithRequestAttributeGetter extracts additional attributes from the request.
+func WithRequestAttributeGetter(fn func(req *http.Request) []attribute.KeyValue) Option {
+	return optionFunc(func(o *config) {
+		o.GetRequestAttributes = fn
+	})
+}
+
+// WithResponseAttributeGetter extracts additional attributes from the response.
+func WithResponseAttributeGetter(fn func(req *http.Response) []attribute.KeyValue) Option {
+	return optionFunc(func(o *config) {
+		o.GetResponseAttributes = fn
+	})
+}


People may NOT want to add always the same attributes to spans and metrics. While for spans it is not a problem for metrics it leads to high cardinality.

I strongly suggest to remove this functionality from this PR. We can create a different solution in a separate PR.

pellared · 2023-08-10T08:39:59Z

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

+	}
+	metricdatatest.AssertEqual(t, want, sm.Metrics[1], metricdatatest.IgnoreTimestamp())
+
+	// Duration value is not predictable.


We can check that it is "non-zero" 😉

Would adding a IgnoreValue option to metricdatatest help?

I created a PR in the main repo to add this "IgnoreValue" option.

pellared · 2023-08-10T08:44:59Z

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

+	reader := metric.NewManualReader()
+	meterProvider := metric.NewMeterProvider(metric.WithReader(reader))


Are the changes needed in this test?

no, will remove.

pellared · 2023-08-10T08:45:45Z

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

 	"go.opentelemetry.io/otel/trace"
 )

+func assertClientScopeMetrics(t *testing.T, sm metricdata.ScopeMetrics, attrs attribute.Set) {


nit: I think we usually add helper functions at the bottom of the file (below tests).

pellared · 2023-08-10T08:49:00Z

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

+	want = metricdata.Metrics{
+		Name: "http.client.response_content_length",
+		Data: metricdata.Sum[int64]{
+			DataPoints:  []metricdata.DataPoint[int64]{{Attributes: attrs, Value: 13}},


This value is tightly coupled to the test where the function is used. Maybe it would be better just to inline the function into the test as right now it is not really reusable?

This file is a copy/change from the handler test, maybe it can be better to leave both equals for future changes?

pellared · 2023-08-10T08:52:46Z

instrumentation/net/http/otelhttp/transport.go

+	counters              map[string]metric.Int64Counter
+	valueRecorders        map[string]metric.Float64Histogram


Why are we using maps here? How about simply having the mutiple instrument fields (many metric.Int64Counter and metric.Float64Histogram fields instead of these two maps).

Also the same thing done by the server handler, this pattern seems to be used in a lot of instrumentations.

Co-authored-by: Robert Pająk <pellared@hotmail.com>

… otelhttp-transport-metrics

jlordiales · 2023-09-04T19:35:52Z

Curious if this is currently blocked by anything? Would be awesome to have this merged and released when possible

jlordiales · 2023-09-12T21:28:15Z

instrumentation/net/http/otelhttp/transport.go

+	}
+
+	// Add metrics
+	attributes := semconvutil.HTTPClientRequest(r)


This probably needs something similar to

opentelemetry-go-contrib/instrumentation/net/http/otelhttp/internal/semconvutil/httpconv.go

Line 106 in fdfa6e3

func HTTPServerRequestMetrics(server string, req *http.Request) []attribute.KeyValue {

which avoids any high cardinality attributes for metrics

jdef · 2023-09-21T20:38:49Z

instrumentation/net/http/otelhttp/transport.go

+	o := metric.WithAttributes(attributes...)
+	t.counters[ClientRequestContentLength].Add(ctx, bw.read.Load(), o)
+	if err == nil {
+		t.counters[ClientResponseContentLength].Add(ctx, res.ContentLength, o)


ContentLength may not be what we want here, could be negative. since bodyWrapper already tracks count of bytes read from the response, it's probably better to refer to that count here

[EDIT] I see that newWrappedBody is used instead of bodyWrapper .. so maybe it's not a drop-in replacement. nonetheless we need a more accurate byte count than http.Response.ContentLength is guaranteed to provide.

jdef · 2023-09-21T20:50:18Z

instrumentation/net/http/otelhttp/transport.go

@@ -109,21 +140,64 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) {
 		ctx = httptrace.WithClientTrace(ctx, t.clientTrace(ctx))
 	}

+	readRecordFunc := func(int64) {}
+	if t.readEvent {


readEvent and readRecordFunc map to "read" in the span .. but it's the client's Request object, so it's actually a "write" event - right? because it's being written to the server?

"read" events should be recorded from the bytes consumed by the http.Response.Body, right?

Sovietaced · 2023-12-12T19:58:17Z

@RangelReale What is the latest on this? Do you need any help?

RangelReale · 2023-12-15T11:00:56Z

@RangelReale What is the latest on this? Do you need any help?

@Sovietaced I've not been following too much the latest changes, if you want to help/assume this fork, it is fine with me.

Sovietaced · 2023-12-17T04:27:26Z

@Sovietaced I've not been following too much the latest changes, if you want to help/assume this fork, it is fine with me.

Thanks! I have made a fork and will be going through the code tomorrow. I will open up another pull request once I deeply understand the code and have addressed any outstanding comments here.

Sovietaced · 2023-12-17T21:20:30Z

I have an initial draft of this work continued here: #4707

Would appreciate any reviews

RangelReale added 2 commits May 3, 2023 08:48

otelhttp transport metrics

2a075fd

get attributes

9710398

RangelReale requested review from a team, Aneurysm9 and dmathieu as code owners May 3, 2023 13:00

RangelReale added 2 commits May 4, 2023 09:11

changelog

706a58a

add transport test

0c1ecad

fix lint

6c046de

youngbupark mentioned this pull request May 4, 2023

[Metrics] Add dependency service metrics radius-project/radius#6319

Open

pellared reviewed May 8, 2023

View reviewed changes

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

RangelReale added 2 commits May 8, 2023 15:43

try data race fix

faed864

use atomic instead of mutex

9304781

forestsword reviewed May 9, 2023

View reviewed changes

RangelReale added 4 commits May 10, 2023 08:51

fix grammar

f66d205

use semconv for http response

0d2323b

don't add status code if error

d073e3c

remove unused import

3602fb7

Merge branch 'main' into otelhttp-transport-metrics

50b6074

spy16 reviewed Jul 28, 2023

View reviewed changes

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

spy16 reviewed Jul 28, 2023

View reviewed changes

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

RangelReale and others added 2 commits July 29, 2023 09:49

fix conflicts

b4716e0

Merge branch 'main' into otelhttp-transport-metrics

4e23144

pellared added 2 commits July 31, 2023 13:00

Update CHANGELOG.md

250299e

Update CHANGELOG.md

f93b5c1

pellared requested changes Jul 31, 2023

View reviewed changes

RangelReale added 4 commits August 8, 2023 12:25

fix lint

bf3503f

Merge branch 'main' into otelhttp-transport-metrics

eafcc75

fix changelog

9fc2629

remove check when error

359859c

RangelReale requested a review from pellared August 8, 2023 16:04

Update CHANGELOG.md

45ceb1d

pellared requested changes Aug 10, 2023

View reviewed changes

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

165ee4b

Co-authored-by: Robert Pająk <pellared@hotmail.com>

RangelReale mentioned this pull request Aug 15, 2023

Ignore value option for metricdatatest open-telemetry/opentelemetry-go#4447

Merged

RangelReale added 4 commits August 15, 2023 11:50

review fixes

09066a8

Merge remote-tracking branch 'origin/otelhttp-transport-metrics' into…

75c7903

… otelhttp-transport-metrics

Merge branch 'main' into otelhttp-transport-metrics

4df192e

Merge branch 'main' into otelhttp-transport-metrics

e28520c

jlordiales reviewed Sep 12, 2023

View reviewed changes

jdef suggested changes Sep 21, 2023

View reviewed changes

jdef reviewed Sep 21, 2023

View reviewed changes

jlordiales mentioned this pull request Sep 25, 2023

Add metrics for transport layer in instrumentation net/http #3134

Closed

evankanderson mentioned this pull request Nov 1, 2023

Add metrics for outbound API calls stacklok/minder#1360

Merged

Harnoor7 mentioned this pull request Nov 15, 2023

OpenTelemetry Support krakend/krakend-ce#478

Closed

Sovietaced mentioned this pull request Dec 17, 2023

otelhttp: client metrics #4707

Merged

		reader := metric.NewManualReader()
		meterProvider := metric.NewMeterProvider(metric.WithReader(reader))

		counters map[string]metric.Int64Counter
		valueRecorders map[string]metric.Float64Histogram

[otelhttp] transport metrics #3769

Are you sure you want to change the base?

[otelhttp] transport metrics #3769

Conversation

RangelReale commented May 3, 2023 • edited by pellared

codecov bot commented May 4, 2023 • edited

Codecov Report

dmathieu commented May 4, 2023

RangelReale commented May 4, 2023

RangelReale commented May 4, 2023

pellared commented May 8, 2023 • edited

forestsword left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pellared commented Jul 27, 2023

RangelReale commented Jul 27, 2023

pellared commented Jul 27, 2023

pellared left a comment • edited

Choose a reason for hiding this comment

pellared commented Aug 10, 2023

pellared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlordiales commented Sep 4, 2023

jlordiales Sep 12, 2023 • edited

Choose a reason for hiding this comment

jdef Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sovietaced commented Dec 12, 2023

RangelReale commented Dec 15, 2023

Sovietaced commented Dec 17, 2023

Sovietaced commented Dec 17, 2023

RangelReale commented May 3, 2023 •

edited by pellared

codecov bot commented May 4, 2023 •

edited

pellared commented May 8, 2023 •

edited

pellared left a comment •

edited

jlordiales Sep 12, 2023 •

edited

jdef Sep 21, 2023 •

edited