Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

only require userland CLS when needed #435

Merged
merged 1 commit into from
Mar 21, 2019
Merged

only require userland CLS when needed #435

merged 1 commit into from
Mar 21, 2019

Conversation

vmarchaud
Copy link
Contributor

@vmarchaud vmarchaud commented Mar 20, 2019

We shipped our APM with the @opencensus/core package. We didn't enable the tracer by default but since when required, it will require the continuation-local-storage which will apply patches automatically. So even if you use async_hooks, you will have the patchs from the userland CLS.
I believe it's safe to change this behavior since it will work the same way.

I would like to raise also the fact that we could only require both CLS implementation when the tracing is enabled to decrease the performance impact when not enabled.

If you are interested, here are the different complaints from users: keymetrics/pm2-io-apm#235

@mayurkale22
Copy link
Member

Please rebase the branch to fix build.

@codecov-io
Copy link

Codecov Report

Merging #435 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #435      +/-   ##
==========================================
- Coverage   95.01%   94.98%   -0.04%     
==========================================
  Files         147      147              
  Lines        9545     9544       -1     
  Branches      679      679              
==========================================
- Hits         9069     9065       -4     
- Misses        476      479       +3
Impacted Files Coverage Δ
src/stackdriver-monitoring.ts 80.2% <0%> (-3.13%) ⬇️
src/internal/cls.ts 81.81% <0%> (-1.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04f1fee...a71acb8. Read the comment docs.

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

LGTM

@mayurkale22 mayurkale22 merged commit 4eaf539 into census-instrumentation:master Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants