Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit traceparent header in gateway responses and add sharness test #9180

Closed
wants to merge 5 commits into from
Closed

Emit traceparent header in gateway responses and add sharness test #9180

wants to merge 5 commits into from

Conversation

iand
Copy link
Contributor

@iand iand commented Aug 11, 2022

The OTel handler extracts incoming trace headers but does not emit them in responses. This has to be done separately and I chose to add it in our gateway handler. The sharness test verifies that no trace header is emitted unless a trace header was sent with the request, that the emitted header keeps the same trace id and that the trace id is collected in traces.

Note that this is to merge into #9169

guseggert and others added 2 commits August 5, 2022 08:17
This should enable trace context propagation from headers in the HTTP
Gateway and APIs.
@@ -179,6 +179,7 @@ require (
github.com/raulk/go-watchdog v1.2.0 // indirect
github.com/spacemonkeygo/spacelog v0.0.0-20180420211403-2296661a0572 // indirect
github.com/spaolacci/murmur3 v1.1.0 // indirect
github.com/stretchr/objx v0.4.0 // indirect
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to avoid a failure in the gotest circleci step that checks whether kubo-as-a-library builds with the current version. Error was

github.com/ipfs/kubo/examples/kubo-as-a-library imports
	github.com/ipfs/kubo/core/coreapi imports
	github.com/ipfs/kubo/tracing imports
	go.opentelemetry.io/otel/exporters/jaeger tested by
	go.opentelemetry.io/otel/exporters/jaeger.test imports
	github.com/stretchr/testify/mock imports
	github.com/stretchr/objx loaded from github.com/stretchr/objx@v0.1.1,
	but go 1.16 would select v0.4.0

@iand
Copy link
Contributor Author

iand commented Aug 16, 2022

go-ipfs-http-client failure looks like an unrelated flake

@iand
Copy link
Contributor Author

iand commented Aug 16, 2022

sharness failure is in test/sharness/t0310-tracing.sh which is the existing tracing test

@Jorropo Jorropo self-requested a review March 28, 2023 14:04
@BigLep
Copy link
Contributor

BigLep commented Mar 31, 2023

@iand : I saw your mention about engaging on this. I think there is a similar item in bifrost-gateway: ipfs/bifrost-gateway#68 . Ultimately I assume there will be some shared code in Boxo.

Comment on lines +297 to +300
// Emit tracing context headers if present
prop := otel.GetTextMapPropagator()
prop.Inject(r.Context(), propagation.HeaderCarrier(w.Header()))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know the difference with:

		gw = otelhttp.NewHandler(gw, "Gateway.Request", otelhttp.WithPropagators(tracing.Propagator()))

There ?

We already use this for cmds and this works great:

cmdHandler = otelhttp.NewHandler(cmdHandler, "corehttp.cmdsHandler", otelhttp.WithPropagators(tracing.Propagator()))

They seems equivalent to me, except otelhttp is cleaner and more descriptive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but it didn't set the HTTP headers

@hacdias
Copy link
Member

hacdias commented Apr 4, 2023

@iand @Jorropo this will be directly included in #9169.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants