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

refactor: unifying shutdown once with BindOnceFuture #2695

Merged
merged 3 commits into from
Jan 6, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Dec 30, 2021

Which problem is this PR solving?

The procedure of shutdown once is implemented at several places. It may be good to have them all to share an abstract BindOnce structure.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #2695 (2bb3a6e) into main (5f3cbc2) will increase coverage by 0.25%.
The diff coverage is 97.01%.

@@            Coverage Diff             @@
##             main    #2695      +/-   ##
==========================================
+ Coverage   92.57%   92.82%   +0.25%     
==========================================
  Files         151      153       +2     
  Lines        5277     5267      -10     
  Branches     1116     1112       -4     
==========================================
+ Hits         4885     4889       +4     
+ Misses        392      378      -14     
Impacted Files Coverage Δ
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts 94.59% <90.90%> (+3.68%) ⬆️
...dk-trace-base/src/export/BatchSpanProcessorBase.ts 94.38% <92.30%> (+2.80%) ⬆️
...s/exporter-trace-otlp-http/src/OTLPExporterBase.ts 100.00% <100.00%> (+7.84%) ⬆️
packages/opentelemetry-core/src/utils/callback.ts 100.00% <100.00%> (ø)
packages/opentelemetry-core/src/utils/promise.ts 100.00% <100.00%> (ø)
...y-sdk-trace-base/src/export/SimpleSpanProcessor.ts 91.30% <100.00%> (+7.43%) ⬆️

@legendecas legendecas force-pushed the bind-once branch 3 times, most recently from 5fac72e to 76edc67 Compare December 30, 2021 05:47
@legendecas legendecas marked this pull request as ready for review December 30, 2021 06:43
@legendecas legendecas requested a review from a team December 30, 2021 06:43
@vmarchaud vmarchaud added the enhancement New feature or request label Dec 31, 2021
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good! This will definitely be useful in other places as well. 💯

@legendecas legendecas merged commit d61f7be into open-telemetry:main Jan 6, 2022
@legendecas legendecas deleted the bind-once branch January 6, 2022 17:20
rauno56 pushed a commit to rauno56/opentelemetry-js that referenced this pull request Jan 14, 2022
@Flarna
Copy link
Member

Flarna commented Feb 17, 2022

Added breaking label late here. It's breaking because protected _isShutdown: boolean = false; was removed resulting in build errors like this (see e.g. #2788).

Error: packages/exporter-trace-otlp-grpc/src/OTLPExporterNodeBase.ts(78,10): error TS2339: Property '_isShutdown' does not exist on type 'OTLPExporterNodeBase<ExportItem, ServiceRequest>'.

Actually not that an issue as it happened before 1.x was made.

@legendecas
Copy link
Member Author

@Flarna we can add the flag back. Didn't notice it is a protected property rather than a private property.

@Flarna
Copy link
Member

Flarna commented Feb 21, 2022

@legendecas That was not my intention here. It's just about getting it correct into the next release notes/changelog. It happens before 1.x as released so the perfect timing.
In the past we mentioned breaking changes in the readme but not sure if this is still the case.

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.

5 participants