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

unifying shutdown across code base #1439

Merged
merged 13 commits into from
Sep 3, 2020
Merged

Conversation

obecny
Copy link
Member

@obecny obecny commented Aug 17, 2020

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.

… spans, processors, so that the shutdown will be correctly handle in whole pipeline
@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #1439 into master will decrease coverage by 0.29%.
The diff coverage is 80.81%.

@@            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     
Impacted Files Coverage Δ
...emetry-metrics/src/export/ConsoleMetricExporter.ts 58.82% <0.00%> (-3.68%) ⬇️
...s/opentelemetry-metrics/src/export/NoopExporter.ts 50.00% <0.00%> (-16.67%) ⬇️
packages/opentelemetry-metrics/src/export/types.ts 100.00% <ø> (ø)
...ges/opentelemetry-tracing/src/NoopSpanProcessor.ts 71.42% <50.00%> (ø)
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts 86.07% <72.00%> (-8.76%) ⬇️
...elemetry-tracing/src/export/SimpleSpanProcessor.ts 84.00% <75.00%> (-1.00%) ⬇️
packages/opentelemetry-metrics/src/Meter.ts 94.30% <76.92%> (-2.12%) ⬇️
...ry-exporter-collector/src/CollectorExporterBase.ts 92.00% <80.00%> (-2.74%) ⬇️
...telemetry-tracing/src/export/BatchSpanProcessor.ts 91.93% <80.76%> (-0.52%) ⬇️
...ackages/opentelemetry-metrics/src/MeterProvider.ts 90.47% <81.81%> (-6.08%) ⬇️
... and 8 more

Copy link
Member

@dyladan dyladan left a 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.

@obecny
Copy link
Member Author

obecny commented Aug 18, 2020

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.

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.

@dyladan
Copy link
Member

dyladan commented Aug 18, 2020

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.

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.

@dyladan
Copy link
Member

dyladan commented Aug 26, 2020

@open-telemetry/javascript-approvers Please take a look at this PR. It is a follow up from Jonah's "graceful shutdown" PR

@dyladan dyladan added the enhancement New feature or request label Sep 3, 2020
Copy link
Member

@markwolff markwolff left a comment

Choose a reason for hiding this comment

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

Looks great!

packages/opentelemetry-metrics/package.json Outdated Show resolved Hide resolved
@obecny obecny merged commit d8b1be8 into open-telemetry:master Sep 3, 2020
@obecny obecny deleted the shutdown_fix branch September 3, 2020 18:30
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…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 <[email protected]>
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 13, 2024
…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 <[email protected]>
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 16, 2024
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants