-
Notifications
You must be signed in to change notification settings - Fork 805
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
unifying shutdown across code base #1439
Conversation
… spans, processors, so that the shutdown will be correctly handle in whole pipeline
Codecov Report
@@ Coverage Diff @@
## master #1439 +/- ##
==========================================
- Coverage 94.11% 93.81% -0.30%
==========================================
Files 153 153
Lines 4671 4756 +85
Branches 959 951 -8
==========================================
+ Hits 4396 4462 +66
- Misses 275 294 +19
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general there are many functions which return promise and could benefit from using async/await. If the purpose of the PR is to unify the APIs, then we should move to the more modern solution.
packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts
Show resolved
Hide resolved
async is not supported on web and this will always be converted into fake generators so the number of code will be significant bigger then just a promise. Also you don't need to always use async - just because it is newer doesn't mean it can / should be used always, specially when you run the same async tasks in parallel instead of trying to wait for each task before running next one etc. |
Ok that's fine. I didn't mean to imply that newer is always better, I just happen to find async/await style significantly more readable. The argument about generators I do understand and appreciate though. |
@open-telemetry/javascript-approvers Please take a look at this PR. It is a follow up from Jonah's "graceful shutdown" PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
…try#1439) * fix(eslint-config): replace gts with prettier and eslint recommended config * fix(eslint-config): using the core repo's configuration --------- Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
…try#1439) * fix(eslint-config): replace gts with prettier and eslint recommended config * fix(eslint-config): using the core repo's configuration --------- Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
…try#1439) * fix(eslint-config): replace gts with prettier and eslint recommended config * fix(eslint-config): using the core repo's configuration --------- Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
This PR is a follow up on graceful shutdown. What it does it is fixing and unifying the shutdown across all exporters, metric, spans, processors, so that the shutdown will be correctly handle in whole pipeline. It will also return promise that will resolve correctly when all tasks that depends on it will resolve including http or grpc request.