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

fix: isolate binding EventEmitter #1937

Merged
merged 6 commits into from
Feb 22, 2021

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Feb 16, 2021

Which problem is this PR solving?

Binding an EventEmitter a second time via another instances of ContextManager shouldn't have side effects to the first ContextManager.

Short description of the changes

Use an unique symbol per ContextManager instance to isolate them.

Binding an EventEmitter a second time via another instances of ContextManager
shouldn't have side effects to the first ContextManager.

Use an unique symbol per ContextManager instance to isolate them.
@codecov
Copy link

codecov bot commented Feb 16, 2021

Codecov Report

Merging #1937 (8c80ee1) into main (38d1ee2) will increase coverage by 0.12%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##             main    #1937      +/-   ##
==========================================
+ Coverage   92.76%   92.88%   +0.12%     
==========================================
  Files         174      174              
  Lines        6108     6061      -47     
  Branches     1265     1250      -15     
==========================================
- Hits         5666     5630      -36     
+ Misses        442      431      -11     
Impacted Files Coverage Δ
...sync-hooks/src/AbstractAsyncHooksContextManager.ts 96.87% <96.42%> (-1.24%) ⬇️
...async-hooks/src/AsyncLocalStorageContextManager.ts 100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...etry-exporter-prometheus/src/PrometheusExporter.ts 90.66% <0.00%> (-1.34%) ⬇️
...ages/opentelemetry-exporter-collector/src/types.ts
...ry-exporter-collector/src/CollectorExporterBase.ts
.../opentelemetry-exporter-collector/src/transform.ts
...lemetry-exporter-collector/src/transformMetrics.ts
...kages/opentelemetry-exporter-collector/src/util.ts
...ntelemetry-web/src/enums/PerformanceTimingNames.ts 100.00% <0.00%> (ø)
... and 5 more

@dyladan dyladan added the enhancement New feature or request label Feb 17, 2021
@Flarna

This comment has been minimized.

@dyladan dyladan added bug Something isn't working and removed enhancement New feature or request labels Feb 17, 2021
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

some minor things

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

approving, the last thing is minor anyway leaving this up to you

@Flarna Flarna merged commit 9437d7e into open-telemetry:main Feb 22, 2021
@Flarna Flarna deleted the async-hooks-no-leak branch February 22, 2021 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants