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

feat: set default OTel propagator #9169

Closed
wants to merge 1 commit into from
Closed

Conversation

guseggert
Copy link
Contributor

@guseggert guseggert commented Aug 5, 2022

This should enable trace context propagation from headers in the HTTP Gateway and APIs.

I think this should have a sharness test to ensure propagation doesn't break, so I'll leave this in draft for now.

Superseds #9180

@BigLep
Copy link
Contributor

BigLep commented Mar 19, 2023

@guseggert and @iand : this work and #9169 have beeen open for 6+ months. Is it still relevant?

@guseggert
Copy link
Contributor Author

This was waiting for bifrost to test this on the gateways but they never finished it that integration. Being able to answer "why was a request slow" or "why did this request time out" seems like a useful thing to have.......

@iand
Copy link
Contributor

iand commented Mar 20, 2023

Will definitely be useful in Thunderdome

@guseggert
Copy link
Contributor Author

Yeah maybe we can wire it up in Thunderdome and use that to get this over the line then? Do you have bandwidth for that right now @iand ?

@hacdias hacdias changed the title feat: set default OTel propagator feat: set default OTel propagator, HTTP Trace Context Apr 4, 2023
@hacdias
Copy link
Member

hacdias commented Apr 4, 2023

I repurposed this PR to include ipfs/boxo#256 and the test from #9180. I'll see if the sharness test is properly working, had some issues locally.

@hacdias
Copy link
Member

hacdias commented Apr 5, 2023

I updated this PR. I think it is ready. Before reviewing, please read ipfs/boxo#256 (comment).

@hacdias hacdias marked this pull request as ready for review April 5, 2023 10:06
This should enable trace context propagation from headers in the HTTP
Gateway and APIs.
@@ -106,6 +107,7 @@ func mainRet() (exitCode int) {
}
}()
otel.SetTracerProvider(tp)
otel.SetTextMapPropagator(autoprop.NewTextMapPropagator())
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to understand what this does, because this isn't setuped yet, however http propagation already works for cmds.

Copy link
Member

@hacdias hacdias Apr 5, 2023

Choose a reason for hiding this comment

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

@Jorropo https://pkg.go.dev/go.opentelemetry.io/contrib/propagators/autoprop@v0.40.0#pkg-overview, for more info ask @guseggert since he added this. AFAIK, the main goal is that it parses the ENV variable and has sensible defaults.

However, it might not make sense since we always use tracing.Propagators() explciitly, so we're never using the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't use it, I would say let's not do this. Deadcode is bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little hazy here because this was so long ago, but presumably I was trying to make the propagation user-configurable with OTel standard environment variables. I wouldn't say this is dead code as the intention is to allow users to wire up OTel in their applications, although this could use some tests.

Copy link
Member

Choose a reason for hiding this comment

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

@guseggert @Jorropo I extracted my commit to #9798.

@guseggert if you want to use autoprop, I think you should either use it as the return value for tracing.Propagators, or set it as default and completely remove tracing.Propagators. But that is not in the scope of what I'm trying to achieve here, so I'll mark this again as a draft.

@hacdias hacdias marked this pull request as draft April 6, 2023 10:11
@hacdias hacdias removed their assignment Apr 6, 2023
@hacdias hacdias changed the title feat: set default OTel propagator, HTTP Trace Context feat: set default OTel propagator Apr 6, 2023
@hacdias hacdias removed the request for review from lidel April 6, 2023 10:12
@hacdias
Copy link
Member

hacdias commented Apr 6, 2023

Superseded by #9801

@hacdias hacdias closed this Apr 6, 2023
@hacdias hacdias deleted the feat/otel-propagators branch April 6, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants