opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

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

Open pichlermarc opened this issue 1 year ago • 4 comments

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:

  • [ ] implement #4551
  • [ ] in @opentelmetry/sdk-node, where the automatic pairing from #4551 is implemented, read the env vars from OTEL_METRIC_EXPORT_INTERVAL/OTEL_METRIC_EXPORT_TIMEOUT and configure the auto-paired PeriodicExportingMetricReader accordingly. Use defaults if the env vars are unset.

Additional infomation

Original feature-request:

  • #4562

pichlermarc avatar Apr 23 '24 09:04 pichlermarc

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

aabmass avatar Apr 23 '24 21:04 aabmass

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] avatar Jun 24 '24 06:06 github-actions[bot]

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] avatar Sep 16 '24 06:09 github-actions[bot]

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

pichlermarc avatar Sep 27 '24 12:09 pichlermarc