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

Exporters shutdown() could return a promise #987

Closed
satazor opened this issue Apr 28, 2020 · 8 comments
Closed

Exporters shutdown() could return a promise #987

satazor opened this issue Apr 28, 2020 · 8 comments
Assignees

Comments

@satazor
Copy link
Contributor

satazor commented Apr 28, 2020

Is your feature request related to a problem? Please describe.

I would like to know when exporters, like jaeger, are done flushing after a shutdown(). Currently, the shutdown method returns nothing, and there's no way to effectively know when the shutdown finished (flushed + closed connection).

Describe the solution you'd like

In Node.js is quite hard to implement a synchronous shutdown() method. One way would be for it to return a promise that fulfils when the shutdown is done (flushed + closed connection)..

Describe alternatives you've considered
N/A.

Additional context
N/A.

@dyladan
Copy link
Member

dyladan commented Apr 28, 2020

I think this is a good idea and it seems like the spec allows for it. Would you like to make the PR or should I mark this as up for grabs?

@satazor
Copy link
Contributor Author

satazor commented Apr 28, 2020

@dyladan I was looking at the code, and found this: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-exporter-prometheus/src/prometheus.ts#L112

It seems that the prometheus exporter already has an optional callback for the shutdown method. I would prefer to use promises as we can leverage async await more easily.

Can we agree on shutdown() => Promise for all exporters?

@dyladan
Copy link
Member

dyladan commented Apr 28, 2020

I am ok with it but I'd like to get @mayurkale22 opinion

@mayurkale22
Copy link
Member

SGTM

@dyladan
Copy link
Member

dyladan commented Apr 28, 2020

@satazor I am going to assign this issue to you. Let me know if that doesn't work for you or if you have any questions.

@satazor
Copy link
Contributor Author

satazor commented Apr 28, 2020

@dyladan alright, I'm going to try and find some time to do this. Do you expect a single PR that implements it on all packages or can I do them separately?

@dyladan
Copy link
Member

dyladan commented Apr 28, 2020

There are only a small number of packages so I would say it can be done all in a single PR. If you do attempt to split it into multiple PRs, take note that we will not merge any PRs that break the build. Updating the interface but not the classes will break the build.

@legendecas
Copy link
Member

Closing as both SpanExporter.shutdown and PushMetricExporter.shutdown returns a promise now.

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

No branches or pull requests

4 participants