opentelemetry-js
opentelemetry-js copied to clipboard
feat(sdk-node): configure trace exporter with environment variables
Which problem is this PR solving?
Spec says a user can set an exporter via environment variable with OTEL_TRACES_EXPORTER
and specify otlp protocol with OTEL_EXPORTER_OTLP_PROTOCOL
or signal specific protocol.
This exporter configuration has been added to the sdk-node
package and only applies when the user hasn't set up an exporter or span processor with code (in their tracing.js
file).
If a user doesn't set up an exporter in code or a span processor in code or doesn't provide a custom exporter via env vars, the sdk-node will set up a default otlp
exporter with a http/protobuf
protocol.
Fixes #2873
Short description of the changes
- Added trace exporter env vars and protocol env vars to
environment.ts
file - Added code configuring the trace exporter in the
sdk-node
package insdk.ts
file
Type of change
Please delete options that are not relevant.
- [x] New feature (non-breaking change which adds functionality)
- [x] This change requires a documentation update
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
- [x] Unit tests
- [x] Integration tests
Checklist:
- [x] Followed the style guidelines of this project
- [x] Unit tests have been added
- [x] Documentation has been updated
ToDo
- [x] Clarify use of
_buildExporterFromEnv()
in the BasicTracerProvider
Codecov Report
Merging #3143 (efa2b91) into main (afe0657) will decrease coverage by
0.73%
. The diff coverage isn/a
.
:exclamation: Current head efa2b91 differs from pull request most recent head 7db936e. Consider uploading reports for the commit 7db936e to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## main #3143 +/- ##
==========================================
- Coverage 93.36% 92.63% -0.74%
==========================================
Files 240 130 -110
Lines 7177 3124 -4053
Branches 1487 656 -831
==========================================
- Hits 6701 2894 -3807
+ Misses 476 230 -246
Impacted Files | Coverage Δ | |
---|---|---|
...ckages/opentelemetry-core/src/utils/environment.ts | 90.90% <ø> (ø) |
|
api/src/platform/node/globalThis.ts | 0.00% <0.00%> (-100.00%) |
:arrow_down: |
...s/opentelemetry-core/src/platform/node/sdk-info.ts | 0.00% <0.00%> (-100.00%) |
:arrow_down: |
...opentelemetry-core/src/platform/node/globalThis.ts | 0.00% <0.00%> (-100.00%) |
:arrow_down: |
...pentelemetry-core/src/platform/node/performance.ts | 0.00% <0.00%> (-100.00%) |
:arrow_down: |
.../packages/api-logs/src/platform/node/globalThis.ts | 0.00% <0.00%> (-100.00%) |
:arrow_down: |
...emetry-api-metrics/src/platform/node/globalThis.ts | 0.00% <0.00%> (-100.00%) |
:arrow_down: |
...lemetry-resources/src/detectors/ProcessDetector.ts | 31.81% <0.00%> (-68.19%) |
:arrow_down: |
...perimental/packages/otlp-exporter-base/src/util.ts | 79.41% <0.00%> (-17.65%) |
:arrow_down: |
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts | 84.48% <0.00%> (-15.52%) |
:arrow_down: |
... and 113 more |
I have a question I wanted to ask @open-telemetry/javascript-approvers and @open-telemetry/javascript-maintainers before I can change this draft pr to ready for review.
I want to remove the following code since it looks like it was an older attempt at configuring exporters from env:
But I'm not 100% sure if this code is necessary here (especially items 2 and 3 above) and if it'll cause breaking changes if I remove that code. Any info/details you can share with me would be much appreciated!
I want to remove the following code since it looks like it was an older attempt at configuring exporters from env:
But I'm not 100% sure if this code is necessary here (especially items 2 and 3 above) and if it'll cause breaking changes if I remove that code. Any info/details you can share with me would be much appreciated!
The (2) is to build a common span processor implementation for spans to send their start/end events. I don't think it should be removed.
(1) and (2) are built upon the registration map _registeredPropagators
and _registeredExporters
on the subclasses of BasicTracerProvider, like https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-node/src/NodeTracerProvider.ts#L38. Is it feasible to configure the OTLP exporter based on that mechanism?
Is it feasible to configure the OTLP exporter based on that mechanism?
Adding on this, this is definitly for this use case that i implemented this back then so that would be cleaner. The way it was supposed to be used is that the sdk extends the node tracer provider with its own exporters/propagators that can then be configured by the env
Ready for review. Some tests are failing because I renamed a file. Do the file names get cached somewhere?
I've tested using this new feature locally with a sample app as well and it works as expected.
i'm not sure about the CI failure, the cache would maybe create failure for 3rd party modules but i find it weird that its modifying the code that is being checked out, did you try building from a fresh install ?
I tried fresh install and it didn't fix it. Then I realized that the file named was not updated on github even though I updated the file name locally and push it up to this branch. So I had to manually update the file name on github and it works now.
I tried fresh install and it didn't fix it. Then I realized that the file named was not updated on github even though I updated the file name locally and push it up to this branch. So I had to manually update the file name on github and it works now.
On systems that are case insensitive like windows or macOS, git can not recognize file renames that are just changing cases.
Sorry for the delay on this. I was traveling for work.
I've refactored the code to set up the span processors in the TracerProviderWithEnvExporters
constructor and updated/added tests (as mentioned in the feedback above).
However, I have a question regarding this code:
- Am I using the
NoopSpanProcessor
correctly here?
I'm trying to only call register
when/if TracerProviderWithEnvExporters
or NodeSDK
set up any span processors and there is a situation in the TracerProviderWithEnvExporters
where there are no span processors set up if a user provided incorrect env values or they chose not set up any exporters. In that case, a NoopSpanProcessor is setup (in the inherited BasicTracerProvider
class I believe). If I call register
without using an if statement then some of existing tests setup in the past don't pass such as the checks around the context manager found here.
- Is it worth adding a comment above line 216 that was mentioned in item 1 above that explains the purpose of checking if a tracer provide has an
NoopSpanProcessor
?
I've been looking into the code in a bit more detail regarding your questions @svetlanabrennan :slightly_smiling_face:
As far as I can see, we could override the register()
method in the TracerProviderWithEnvExporters
class. Then, we can only call the superclass's register()
method when SpanProcessor
have been added. We can also override the addSpanProvider()
method, call super.addSpanProvider()
to keep track of any calls to that method.
// part of TracerProviderWithEnvExporters
override addSpanProcessor(spanProcessor: SpanProcessor) {
super.addSpanProcessor(spanProcessor);
this._hasSpanProcessors = true;
}
override register(config?: SDKRegistrationConfig) {
if (this._hasSpanProcessors) {
super.register(config);
}
}
If I am not mistaken, this way it should be possible to call register()
without checking for NoopSpanProcessor
. :slightly_smiling_face:
I've been looking into the code in a bit more detail regarding your questions @svetlanabrennan 🙂
As far as I can see, we could override the
register()
method in theTracerProviderWithEnvExporters
class. Then, we can only call the superclass'sregister()
method whenSpanProcessor
have been added. We can also override theaddSpanProvider()
method, callsuper.addSpanProvider()
to keep track of any calls to that method.// part of TracerProviderWithEnvExporters override addSpanProcessor(spanProcessor: SpanProcessor) { super.addSpanProcessor(spanProcessor); this._hasSpanProcessors = true; } override register(config?: SDKRegistrationConfig) { if (this._hasSpanProcessors) { super.register(config); } }
If I am not mistaken, this way it should be possible to call
register()
without checking forNoopSpanProcessor
. 🙂
This is great feedback. This does remove the need to check for the NoopSpanProcessor
. Thank you.
I think just overriding the register
method will work and no need to override the addSpanProcessor
method because I can use the spanProcessors
property to check if any span processors were added before calling register
if a selected tracer provider is TracerProviderWithEnvExporters
, like this:
// in the TracerProviderWithEnvExporters
override register(config?: SDKRegistrationConfig) {
if (this._spanProcessors) {
super.register(config);
}
}