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

buildkitd: configure telemetry providers with the global handlers #4957

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsternberg
Copy link
Collaborator

The global handlers are used as defaults for a bunch of OTEL functions and we'll likely use it that way in the future too. While it's always helpful to have a way to manually set the providers (and not rely on the global providers), setting the global providers from the main function is generally the correct thing to do.

This also removes the need for us to use the WithTracerProvider and WithMeterProvider option to the grpc stats handler as it will use the global configuration if this option isn't set. Since this code is in the main function too, it's safe to rely on that behavior.

The global handlers are used as defaults for a bunch of OTEL functions
and we'll likely use it that way in the future too. While it's always
helpful to have a way to manually set the providers (and not rely on the
global providers), setting the global providers from the main function
is generally the correct thing to do.

This also removes the need for us to use the `WithTracerProvider` and
`WithMeterProvider` option to the grpc stats handler as it will use the
global configuration if this option isn't set. Since this code is in the
main function too, it's safe to rely on that behavior.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

I don't think we should add this. At least for the traces. The tracing context contains the parent definition and the tracer definition. If it is missing for some reason then the span would be created with wrong parent as well in wrong position of the trace that we want to avoid. We can always get correct one from context so don't need additional method.

This also removes the need for us to use the WithTracerProvider and WithMeterProvider option to the grpc stats handler as it will use the global configuration if this option isn't set.

Same could be said for any other function parameter where instead of passing the config packages could communicate via both reading/writing same public global variable. But it is considered a code smell in Go and at least should not be used in reusable library packages. I don't see a big reason why we should completely limit ourself to single tracerprovider, if not for anything else I could see the multiple being useful at least for tests.

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

2 participants