otelhttp: Record metrics on timed out requests #4634

carrbs · 2023-11-27T22:26:10Z

Copied verbatim the withoutCancel flow (referenced in #4542) and used it to ensure the metrics are recorded even if the parent ctx is closed. This "patch" (as well as the one in: instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go) can be updated to use the context.WithoutContext in Go 1.21 once we are no longer supporting Go 1.20.

…ding metrics

linux-foundation-easycla · 2023-11-27T22:26:14Z

The committers listed above are authorized under a signed CLA.

✅ login: carrbs / name: Benjamin Sanabria Carr (a971219, a52aa2d, 39362c3, 55b12a7, d521689, d6d3728, 4cd08b4, 2a310bd, c5e3cba, 4bde186, ccf1f47, 238bcb6, caf1167, 0fbf817, 85a8e2b)
✅ login: pellared / name: Robert Pająk (caf1167, b76e57b)

pellared

Implementation looks OK.

I suggest adding a test in instrumentation/net/http/otelhttp/test/handler_test.go.

I would try adding a server that would always have a canceled request. I guess that something like this should work:

	// here setup a handler instrumented with otelhttp

	ctx, cancel := context.WithCancel(context.Background())
	cancel()
	l, err := net.Listen("tcp", "localhost:0")
	require.NoError(t, err)
	srv := &http.Server{
		BaseContext:  func(_ net.Listener) context.Context { return ctx },
		ReadTimeout:  time.Second,
		WriteTimeout: 10 * time.Second,
		Handler:      otelHandler,
	}
	go func() {
		// When Shutdown is called, Serve immediately returns ErrServerClosed.
		assert.Equal(t, http.ErrServerClosed, srv.Serve(l))
	}()
	t.Cleanup(func() {
		assert.NoError(t, srv.Shutdown(context.Background()) 
	})

	// here make a call using HTTP Client. The address of the server is l.Addr().String()

codecov · 2023-11-28T10:44:34Z

Codecov Report

Merging #4634 (b76e57b) into main (6009b31) will decrease coverage by 0.1%.
The diff coverage is 44.4%.

❗ Current head b76e57b differs from pull request most recent head 85a8e2b. Consider uploading reports for the commit 85a8e2b to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #4634     +/-   ##
=======================================
- Coverage   80.8%   80.7%   -0.1%     
=======================================
  Files        150     150             
  Lines      10374   10389     +15     
=======================================
+ Hits        8388    8393      +5     
- Misses      1844    1853      +9     
- Partials     142     143      +1

Files	Coverage Δ
instrumentation/net/http/otelhttp/handler.go	`83.4% <44.4%> (-4.0%)`	⬇️

…entelemetry-go-contrib into metrics-for-timed-out-requests

carrbs · 2023-12-02T20:10:21Z

Hi @pellared, I attempted to test ctxWithoutCancel with your suggestion, but I think something is amiss either with my request or how I've instrumented the handler. The test passes whether or not ctxWithoutCancel is used. Likely (I hope) it's something obvious you can redirect me with on how I set up the test.

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

…o metrics-for-timed-out-requests

pellared · 2023-12-05T12:32:11Z

instrumentation/net/http/otelhttp/handler.go

+	if ctx.Done() == nil {
+		return ctx
+	}
+	return withoutCancelCtx{ctx}


Why not?

Suggested change

if ctx.Done() == nil {

return ctx

}

return withoutCancelCtx{ctx}

if parent == nil {

panic("cannot create context from nil parent")

}

return withoutCancelCtx{parent}

It is copied from https://cs.opensource.google/go/go/+/refs/tags/go1.21.4:src/context/context.go;l=563-571 therefore I would keep it.

Sounds good, I didn't know this had been copied from this. The thinking was that, if a context that wasn't cancellable was passed to withoutCancel it seemed like we could return/no-op (and, if it was nil, we'd get a panic).

pellared · 2023-12-05T12:32:32Z

@carrbs Has my suggestion helped?

carrbs · 2023-12-05T15:58:07Z

@pellared, yes, thank you! I'd overlooked that when trying to track down how to test this. It's passing now and I confirmed
here:
https://github.com/open-telemetry/opentelemetry-go/blob/6cee2b4a4c76b581115d0d0ca150ad8b2e683db6/sdk/metric/instrument.go#L208-L210
and here:
https://github.com/open-telemetry/opentelemetry-go/blob/6cee2b4a4c76b581115d0d0ca150ad8b2e683db6/sdk/metric/instrument.go#L208-L210
The expected behavior of the withoutCancelCtx. I'm gonna clean the PR up and take it out of draft.

instrumentation/net/http/otelhttp/handler.go

… code

pellared · 2023-12-05T17:40:47Z

I'm gonna clean the PR up and take it out of draft.

Please do not forget to add an entry in "Fixed" section in CHANGELOG.md 😉

CHANGELOG.md

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

pellared · 2023-12-05T18:20:58Z

@carrbs Thank you for your contribution. Do not hesitate to leave suggestions that could make future contributions easier.

pellared · 2023-12-05T18:52:34Z

@Aneurysm9 @dmathieu PTAL as code owners.

carrbs · 2023-12-05T19:18:09Z

what's up with the codecov/patch failed test? Is it expecting some unit testing?

pellared · 2023-12-05T19:25:51Z

what's up with the codecov/patch failed test? Is it expecting some unit testing?

It is fine. It just notifies that some new code is not covered by tests which is fine. See: https://app.codecov.io/gh/open-telemetry/opentelemetry-go-contrib/pull/4634?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=open-telemetry

pellared · 2023-12-06T09:39:49Z

The fix would not be needed if open-telemetry/opentelemetry-go#4671 is accepted and merged.

dmathieu · 2023-12-06T17:34:55Z

The SDK native solution seems more elegant, as we wouldn't have to set it up for every instrumentation.

MrAlias · 2023-12-06T17:39:56Z

instrumentation/net/http/otelhttp/handler.go

@@ -281,3 +283,34 @@ func WithRouteTag(route string, h http.Handler) http.Handler {
 		h.ServeHTTP(w, r)
 	})
 }
+
+func withoutCancel(parent context.Context) context.Context {


Please add a note, including a link, about where this is copied from.

Happy to do this, but it sounding like the better option might be the SDK native one (so long as we would not need the flexibility of calling those .Add and .Record methods in such a way where we want different behavior based on the context's cancelled state)?

Added, in case we opt to merge this PR. Thanks @MrAlias!

…entelemetry-go-contrib into metrics-for-timed-out-requests

pellared · 2023-12-08T13:29:25Z

I am changing to draft because of open-telemetry/opentelemetry-go#4671

@carrbs Feel free to review the referenced PR.

pellared · 2023-12-15T18:16:04Z

Closing per open-telemetry/opentelemetry-go#4671

copying withoutContext from otelgrpc stats_handler for use when recor…

a971219

…ding metrics

minor formatting fix

a52aa2d

pellared reviewed Nov 28, 2023

View reviewed changes

carrbs added 5 commits December 2, 2023 11:51

Adds a test to attempt to test ctxWithoutCancel

39362c3

Merge branch 'main' into metrics-for-timed-out-requests

55b12a7

remove misleading comment

d521689

Merge branch 'metrics-for-timed-out-requests' of github.com:carrbs/op…

d6d3728

…entelemetry-go-contrib into metrics-for-timed-out-requests

remove unneeded print statement

4cd08b4

pellared reviewed Dec 4, 2023

View reviewed changes

instrumentation/net/http/otelhttp/test/handler_test.go Outdated Show resolved Hide resolved

carrbs added 3 commits December 4, 2023 20:35

return existing ctx if it can't be canceled

2a310bd

modify test to make a request to the server correctly

c5e3cba

Merge branch 'main' of github.com:carrbs/opentelemetry-go-contrib int…

4bde186

…o metrics-for-timed-out-requests

pellared reviewed Dec 5, 2023

View reviewed changes

carrbs changed the title ~~[WIP] otelhttp: Record metrics on timed out requests by using a context that doesn't cancel~~ otelhttp: Record metrics on timed out requests by using a context that doesn't cancel Dec 5, 2023

carrbs commented Dec 5, 2023

View reviewed changes

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

modify withoutCancel to directly mimic behavior in the context source…

ccf1f47

… code

Update CHANGELOG.md

238bcb6

carrbs marked this pull request as ready for review December 5, 2023 17:53

carrbs requested review from Aneurysm9 and dmathieu as code owners December 5, 2023 17:53

carrbs requested a review from a team December 5, 2023 17:53

pellared approved these changes Dec 5, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md per suggestion

caf1167

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

pellared changed the title ~~otelhttp: Record metrics on timed out requests by using a context that doesn't cancel~~ otelhttp: Record metrics on timed out requests Dec 5, 2023

Update CHANGELOG.md

b76e57b

This was referenced Dec 6, 2023

[exporterhelper] fix missed metric aggregations open-telemetry/opentelemetry-collector#9048

Merged

sdk/metric: Record measurements when context is done open-telemetry/opentelemetry-go#4671

Merged

MrAlias reviewed Dec 6, 2023

View reviewed changes

carrbs added 2 commits December 6, 2023 16:34

Added a note and some links about where this is copied from

0fbf817

Merge branch 'metrics-for-timed-out-requests' of github.com:carrbs/op…

85a8e2b

…entelemetry-go-contrib into metrics-for-timed-out-requests

pellared marked this pull request as draft December 8, 2023 13:29

pellared closed this Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

otelhttp: Record metrics on timed out requests #4634

otelhttp: Record metrics on timed out requests #4634

carrbs commented Nov 27, 2023 •

edited

Loading

linux-foundation-easycla bot commented Nov 27, 2023 •

edited

Loading

pellared left a comment •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

carrbs commented Dec 2, 2023

pellared Dec 5, 2023

carrbs Dec 5, 2023

pellared commented Dec 5, 2023 •

edited

Loading

carrbs commented Dec 5, 2023

pellared commented Dec 5, 2023

pellared commented Dec 5, 2023

pellared commented Dec 5, 2023

carrbs commented Dec 5, 2023

pellared commented Dec 5, 2023

pellared commented Dec 6, 2023 •

edited

Loading

dmathieu commented Dec 6, 2023

MrAlias Dec 6, 2023

carrbs Dec 6, 2023

carrbs Dec 7, 2023

pellared commented Dec 8, 2023

pellared commented Dec 15, 2023

otelhttp: Record metrics on timed out requests #4634

otelhttp: Record metrics on timed out requests #4634

Conversation

carrbs commented Nov 27, 2023 • edited Loading

linux-foundation-easycla bot commented Nov 27, 2023 • edited Loading

pellared left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

carrbs commented Dec 2, 2023

pellared Dec 5, 2023

Choose a reason for hiding this comment

carrbs Dec 5, 2023

Choose a reason for hiding this comment

pellared commented Dec 5, 2023 • edited Loading

carrbs commented Dec 5, 2023

pellared commented Dec 5, 2023

pellared commented Dec 5, 2023

pellared commented Dec 5, 2023

carrbs commented Dec 5, 2023

pellared commented Dec 5, 2023

pellared commented Dec 6, 2023 • edited Loading

dmathieu commented Dec 6, 2023

MrAlias Dec 6, 2023

Choose a reason for hiding this comment

carrbs Dec 6, 2023

Choose a reason for hiding this comment

carrbs Dec 7, 2023

Choose a reason for hiding this comment

pellared commented Dec 8, 2023

pellared commented Dec 15, 2023

carrbs commented Nov 27, 2023 •

edited

Loading

linux-foundation-easycla bot commented Nov 27, 2023 •

edited

Loading

pellared left a comment •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading

pellared commented Dec 5, 2023 •

edited

Loading

pellared commented Dec 6, 2023 •

edited

Loading