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

fix: one exporter to rule them all #243

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

tim-mwangi
Copy link
Collaborator

Description

One exporter for all registered services. We are currently creating a new exporter every time a new service is registered which results in multiple grpc clients to the collector being created.

Testing

Manual tests

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

one exporter for all registered services
}
case config.TraceReporterType_LOGGING:
return func() (sdktrace.SpanExporter, error) {
// TODO: Define if endpoint could be a filepath to write into a file.
return stdouttrace.New(stdouttrace.WithPrettyPrint())
globalExporter, globalExporterErr = stdouttrace.New(stdouttrace.WithPrettyPrint())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be thread safe? If yes, do we need to worry about races resulting in some dangling exporters or that’s not a big deal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe not since it would set up in the Init method. Unless all the consumers do not call Init. But I will shelf this one for now since I did not observe any remarkable improvement with one exporter vs multiple exporters.

Copy link
Collaborator

@ryanericson ryanericson Dec 6, 2024

Choose a reason for hiding this comment

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

Not opposed to shelving it, but why no improvement? This should help to reduce number of export connections?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't have time to verify that using lsof or some other networking tool. I suspect reducing clients does not mean reducing connections as well. The clients maybe fewer but the connection count may still be the same. I would want to verify it next.

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.

2 participants