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

feat(tracing): allow to configure exporter by environment #1676 #2100

Merged
merged 6 commits into from
May 12, 2021

Conversation

vmarchaud
Copy link
Member

@vmarchaud vmarchaud commented Apr 11, 2021

Opening PR for feedback, see #1676 for discussions

Fixes #1676

@vmarchaud

This comment has been minimized.

@vmarchaud vmarchaud closed this May 2, 2021
@vmarchaud vmarchaud reopened this May 2, 2021
@vmarchaud vmarchaud force-pushed the trace-exporter-configuration branch from 9288511 to 75e963b Compare May 2, 2021 14:38
@vmarchaud vmarchaud marked this pull request as ready for review May 2, 2021 14:39
@vmarchaud
Copy link
Member Author

This is the first part to allow tracing SDKs to load exporter via environment, this will allows higher level SDKs that have exporters has dependencies to register them to be loaded via the environment (as discussed in SIG).
I exported the NoopSpanProcessor to be able to test which SpanProcessor is registered on the tracing SDK (since the exporter is configured through a span processor, we must first access the span processor and then the exporter)

@vmarchaud vmarchaud force-pushed the trace-exporter-configuration branch from 75e963b to f87c346 Compare May 2, 2021 14:48
@codecov
Copy link

codecov bot commented May 2, 2021

Codecov Report

Merging #2100 (e7ed31f) into main (02239b5) will increase coverage by 0.02%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main    #2100      +/-   ##
==========================================
+ Coverage   92.71%   92.73%   +0.02%     
==========================================
  Files         141      141              
  Lines        5037     5055      +18     
  Branches     1037     1042       +5     
==========================================
+ Hits         4670     4688      +18     
  Misses        367      367              
Impacted Files Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts 95.83% <ø> (ø)
...ntelemetry-tracing/src/export/NoopSpanProcessor.ts 71.42% <ø> (ø)
...s/opentelemetry-tracing/src/BasicTracerProvider.ts 96.10% <95.00%> (-0.51%) ⬇️
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@Flarna
Copy link
Member

Flarna commented May 2, 2021

A test which actually registers an exporter and uses it via env would be nice.

@vmarchaud vmarchaud force-pushed the trace-exporter-configuration branch from be8a185 to f8e93da Compare May 8, 2021 09:01
@vmarchaud
Copy link
Member Author

A test which actually registers an exporter and uses it via env would be nice.

Added !

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.

one concern with shutdown and 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.

lgtm

@vmarchaud vmarchaud force-pushed the trace-exporter-configuration branch from 05be8ca to 6f63e57 Compare May 11, 2021 18:54
@dyladan dyladan added the enhancement New feature or request label May 12, 2021
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.

Exporter environment configuration
4 participants