Skip to content

Record error.type on failed collections in PeriodicMetricReader#8650

Open
TimurRakhmatullin86 wants to merge 1 commit into
open-telemetry:mainfrom
TimurRakhmatullin86:fix/periodic-metric-reader-error-type
Open

Record error.type on failed collections in PeriodicMetricReader#8650
TimurRakhmatullin86 wants to merge 1 commit into
open-telemetry:mainfrom
TimurRakhmatullin86:fix/periodic-metric-reader-error-type

Conversation

@TimurRakhmatullin86

Copy link
Copy Markdown

Problem

In PeriodicMetricReader.Scheduled.doRun() the self-observability sample for
otel.sdk.metric_reader.collection.duration is always recorded with a null error:

long startNanoTime = CLOCK.nanoTime();
String error = null;
Collection<MetricData> metricData;
try {
  metricData = collectionRegistration.collectAllMetrics();
} finally {
  long durationNanos = CLOCK.nanoTime() - startNanoTime;
  instrumentation.recordCollection(durationNanos / 1_000_000_000.0, error);
}

error is declared, initialized to null, and never assigned anywhere in the method, so
MetricReaderInstrumentation.recordCollection's if (error != null) branch that attaches the
error.type attribute is unreachable from this caller. When collectAllMetrics() throws, the
finally still records the sample — as a successful collection with no error.type — and
the exception propagates to the outer catch (Throwable). The self-observability histogram
therefore reports error-free collections while the reader is exporting nothing, which is
exactly the signal an operator would use to detect the outage.

The sibling BatchSpanProcessor.exportCurrentBatch() uses the same String error = null; … finally { instrumentation…(…, error); } shape but assigns error on each failure path.

Fix

Set error to the throwable's class name before it propagates:

try {
  metricData = collectionRegistration.collectAllMetrics();
} catch (Throwable t) {
  error = t.getClass().getName();
  throw t;
} finally {
  ...
}

The rethrow keeps the existing control flow — the exception still reaches the same outer
catch (Throwable) — so the only change is that the recorded sample now carries error.type.
(ThrowableUtil.propagateIfFatal is intentionally not added: the outer catch already handles
all throwables, so it would be a no-op here.)

Tests

SdkMeterProviderMetricsTest.collectionFailureIsRecordedWithErrorType: registers a
MetricProducer that throws on the first produce() then succeeds, flushes twice, and asserts
the exported otel.sdk.metric_reader.collection.duration point carries
error.type = java.lang.IllegalStateException. It fails against the current code (the point has
no error.type) and passes with the fix. Full :sdk:metrics:test suite and
:sdk:metrics:spotlessCheck pass; errorprone/NullAway clean.

The self-observability sample for otel.sdk.metric_reader.collection.duration
was always recorded with a null error: the local `error` variable was declared
but never assigned, so a failed collectAllMetrics() was reported as a
successful collection (error.type never set) - hiding exactly the failures an
operator watches this signal for.

Set `error` to the throwable's class name before it propagates, mirroring
BatchSpanProcessor.exportCurrentBatch(), so the finally records the failure.

Signed-off-by: Timur Rakhmatullin <174210871+TimurRakhmatullin86@users.noreply.github.com>
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 20, 2026

Copy link
Copy Markdown

Pull request dashboard status

Status last refreshed: 2026-07-21 21:49:28 UTC.

  • Waiting on: Reviewers
  • Next step: Review the latest changes.

This automated status or its linked feedback items may be incorrect. If something looks wrong, report it with the result you expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant