Opentelemetry-js: Exporters shutdown() could return a promise

Created on 28 Apr 2020  路  7Comments  路  Source: open-telemetry/opentelemetry-js

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.

feature-request

Most helpful comment

@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?

All 7 comments

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?

@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?

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

SGTM

@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.

@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?

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.

Was this page helpful?
0 / 5 - 0 ratings