search

open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.74k stars 803 forks source link

[sdk-node] implement enviornment configuration for auto-paired `PeriodicExportingMetricReader` #4655

Open pichlermarc opened 6 months ago

pichlermarc commented 6 months ago

Description

The specification defines that PeriodicExporingMetricReaders that are auto-paired with an exporter can be configured via environment variables.

For this issue to be considered done we need to:

Additional infomation

Original feature-request:

aabmass commented 6 months ago

The specification defines that PeriodicExporingMetricReaders that are auto-paired with an exporter can be configured via environment variables.

Does the spec actually say anything about this only applying when auto-paired? The BatchSpanProcessor reads from the environment variables directly https://github.com/open-telemetry/opentelemetry-js/blob/31eb60dc99dc066cf2085864f2727eb29ee76e91/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts#L55-L58

so the environment variables are applied globally. I'm guessing this is related to the yaml config file conversation and the related concern around precedence

github-actions[bot] commented 4 months ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 1 month ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

pichlermarc commented 1 month ago

The specification defines that PeriodicExporingMetricReaders that are auto-paired with an exporter can be configured via environment variables.

Does the spec actually say anything about this only applying when auto-paired? The BatchSpanProcessor reads from the environment variables directly

https://github.com/open-telemetry/opentelemetry-js/blob/31eb60dc99dc066cf2085864f2727eb29ee76e91/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts#L55-L58

so the environment variables are applied globally. I'm guessing this is related to the yaml config file conversation and the related concern around precedence

That's correct, the spec does not mention anything about auto-pairing. However, we currently don't apply any env var config in the @opentelemetry/sdk-metrics package as we've realized that mixing in env var config with SDK code quickly tends to grow to unmaintainable,very hard-to-debug, and very hard-to-test levels. SDK code for the @opentelemetry/sdk-metrics package is intended to work both in Node.js and the Browser - by adding environment config code that's dead for Browser users, we further increase bundle size which unfortunately is already at unsustainable levels.

In future iterations of the SDK (metrics, traces and logs), we will pull all environment config code into separate files, so that pure SDK packages are free of environment config code. A further challenge will be the introduction of file-config support which will be easier to address when enviornment config is seperate from the actual SDKs