Feature/issue 20 metrics env vars#72
Conversation
…ering Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
Signed-off-by: harshitt13 <find.harshitkushwaha@gmail.com>
| @@ -0,0 +1,32 @@ | |||
| // Licensed under the Apache License, Version 2.0 | |||
| // Copyright 2025, Michael Bushe, All rights reserved. | |||
There was a problem hiding this comment.
Repo convention for new files is the CNCF header (see #64, which converted the whole tree):
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0This package is bound for CNCF donation, so my personal copyright lines - recently deleted - should not exist.
| } | ||
|
|
||
| /// Resolved metrics SDK configuration derived from environment variables. | ||
| class MetricsSdkConfig { |
There was a problem hiding this comment.
A typed config class instead of a Map<String, dynamic> bag — this is exactly the direction the env config should be heading (it's what the #66 review asks for on the BLRP side), nice. Two refinements to finish the shape:
- Move the parsing here as a
MetricsSdkConfig.fromEnvironment()factory (theparseMetricsSdkConfig/_parseMetricsExemplarFilter/_parseDurationMillisecondstrio currently inOTelEnv), following yourBatchSpanProcessorConfig.fromEnvironment()from feat: implement BatchSpanProcessor environment variables #59.OTelEnvstays the raw env-reading layer; the config class owns its defaults and validation. MetricsSdkConfigandMetricsExemplarFilterare returned by a public API but aren't exported fromlib/dartastic_opentelemetry.dart— as written, callers can receive a type they can't name. Add the barrel export (or make the API internal until it's meant to be public).
| case 'trace_based': | ||
| return MetricsExemplarFilter.traceBased; | ||
| default: | ||
| if (OTelLog.isDebug()) { |
There was a problem hiding this comment.
OTelLog.debug hides these from anyone not running with debug diagnostics on — but an operator who typo'd OTEL_METRICS_EXEMPLAR_FILTER=alwayson is exactly the person who needs to hear about it. The spec asks for a warning on unusable values, and the rest of this file warns (getBspConfig, the BLRP work in #66 after review). Please use OTelLog.isWarn()/OTelLog.warn here and in _parseDurationMilliseconds below.
| } | ||
|
|
||
| final metricsSdkConfig = OTelEnv.getMetricsSdkConfig(); | ||
| MetricTransformer.setExemplarFilter(metricsSdkConfig.exemplarFilter); |
There was a problem hiding this comment.
This is the one structural issue in the PR, and it's worth understanding deeply because it's a spec-layering question, not a style one.
Per the spec, the exemplar filter decides whether a measurement is offered to the exemplar reservoir at recording time — it's an SDK collection policy, not an export-time drop. Applying it in the OTLP transformer means (a) we pay the full cost of collecting exemplars and then throw them away at the wire, and (b) other exporters (console, a future second OTLP endpoint) silently share whatever filter the last initialize set, because…
…the mechanism is a mutable global static on MetricTransformer, which is a pure wire encoder today. That global bleeds across meter providers and across tests (the same reason OTelFactory state gets reset so carefully everywhere else in this codebase), and it makes the transformer's output depend on hidden state.
The good news: the right home already exists in this repo. The unconditional exemplars.add(exemplar) calls in lib/src/metrics/storage/gauge_storage.dart and histogram_storage.dart are the spec's "offer" point — gate those on the filter (with trace_based checking the current span context) and the transformer stays a pure encoder. That's how the Java/Go/Python SDKs structure it too. The env parsing and MetricsSdkConfig plumbing in this PR are right and worth keeping as-is; happy to split so interval/timeout land now and the storage-layer filter follows as its own PR if you'd like a checkpoint on the design before writing it.
| static List<proto.Exemplar> _transformExemplars(List<Exemplar>? exemplars) { | ||
| if (exemplars == null || exemplars.isEmpty) { | ||
| return const []; | ||
| } | ||
|
|
||
| return exemplars.where((exemplar) { | ||
| switch (_exemplarFilter) { | ||
| case MetricsExemplarFilter.alwaysOn: | ||
| return true; | ||
| case MetricsExemplarFilter.alwaysOff: | ||
| return false; | ||
| case MetricsExemplarFilter.traceBased: | ||
| return exemplar.traceId != null || exemplar.spanId != null; | ||
| } | ||
| }).map((exemplar) { | ||
| return proto.Exemplar( |
There was a problem hiding this comment.
Three wire-correctness issues in the exemplar encoding itself, independent of where the filter lives:
- The trace context is dropped on the floor:
proto.Exemplaris built with onlytimeUnixNanoandasDouble— notraceId/spanIdbytes, nofilteredAttributes. An exemplar without its trace/span ids can't link a metric point to the trace that produced it, which is the entire point oftrace_basedexemplars. (TraceId.bytes/SpanId.bytesgive you the 16/8-byte values the proto wants.) asDoubleunconditionally coerces int measurements: protoExemplaris a oneof — integer measurements should encode asasInt, doubles asasDouble. Always usingasDoublechanges the wire type for counter exemplars.- The
trace_basedpredicate at line 231:traceId != null || spanId != nullaccepts half-formed context. Spec semantics are "measurements with a sampled trace context" — at minimum this should be&&plus a validity check (traceId.isValid), or exemplars from unsampled/invalid contexts leak through.
Points 1–2 matter even if the filter moves layers, so they're worth fixing here regardless of how the architecture discussion lands.
| metricReader ??= PeriodicExportingMetricReader( | ||
| metricExporter, | ||
| interval: const Duration(seconds: 15), | ||
| interval: metricsSdkConfig.exportInterval, |
There was a problem hiding this comment.
Spec-correct default (OTEL_METRIC_EXPORT_INTERVAL defaults to 60000 ms) — but note this changes the SDK's out-of-the-box behavior from the previous hardcoded 15 s to 60 s, so it must be called out in the CHANGELOG under ## [1.1.0-beta.10-wip] with the "set OTEL_METRIC_EXPORT_INTERVAL=15000 to restore the old cadence" escape hatch. There's precedent wording in the beta.8 notes for the BSP 1 s → 5 s change. (The PR currently has no CHANGELOG entry at all — please add one covering all three new vars.)
| } | ||
|
|
||
| /// How often metrics are exported. | ||
| Duration get interval => _interval; |
There was a problem hiding this comment.
Exposing interval/timeout as getters is the right call — it's what makes the pipeline shape assertable in tests instead of only observable through timing, same pattern the exporter-selection tests rely on elsewhere. 👍
Closes #71