Skip to content

PeriodicExportingMetricReader: timeout can be greater than interval #5550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sburuiana-snyk opened this issue Mar 20, 2025 · 1 comment · Fixed by #5621 · May be fixed by #5730
Open

PeriodicExportingMetricReader: timeout can be greater than interval #5550

sburuiana-snyk opened this issue Mar 20, 2025 · 1 comment · Fixed by #5621 · May be fixed by #5730
Labels
bug Something isn't working pkg:sdk-metrics priority:p3 Bugs which cause problems in user apps not related to correctness (performance, memory use, etc)

Comments

@sburuiana-snyk
Copy link

sburuiana-snyk commented Mar 20, 2025

What happened?

A PeriodicExportingMetricReader has a _exportInterval and a _exportTimeout, that can optionally be specified via options in the constructor. If not specified, they will default to 60s and 30s respectively, as per otel documentation.

This line in the reader code suggests that the intention is that timeout should never be greater than interval (which does make sense from a logic standpoint), and the reader will throw an error if values are specified as such in the constructor.

However, if just one of the two values is specified (and the other is implicitly set to the default value), it can lead to a situation where the timeout does become greater than the interval. For example, consider this snippet:

const periodicMetricReader = new PeriodicExportingMetricReader({
  exporter: this.metricExporter,
  exportIntervalMillis: 10000,
});

This will lead to the reader having _exportInterval == 10000 and _exportTimeout == 30000, which seems unintended based on the previously mentioned constructor behavior.

A quick suggestion for a fix would be to move the check a couple lines below:

-    if (
-     options.exportTimeoutMillis !== undefined &&
-      options.exportIntervalMillis !== undefined &&
-      options.exportIntervalMillis < options.exportTimeoutMillis
-    ) {
-      throw Error(
-        'exportIntervalMillis must be greater than or equal to exportTimeoutMillis'
-      );
-    }

    this._exportInterval = options.exportIntervalMillis ?? 60000;
    this._exportTimeout = options.exportTimeoutMillis ?? 30000;

+    if (this._exportInterval < this._exportTimeout) {
+      throw Error(
+        'export interval must be greater than or equal to export timeout'
+      );
+    }

or maybe just reject the constructor if one of the values is specified and not the other. I'm not sure what the intended resulting behavior would need to be, so I'm opening this issue instead of opening a PR.

OpenTelemetry Setup Code

Irrelevant for this issue

package.json

Irrelevant for this issue

Relevant log output

Irrelevant for this issue

Operating System and Version

Irrelevant for this issue

Runtime and Version

Irrelevant for this issue

@sburuiana-snyk sburuiana-snyk added bug Something isn't working triage labels Mar 20, 2025
@pichlermarc pichlermarc added pkg:sdk-metrics priority:p3 Bugs which cause problems in user apps not related to correctness (performance, memory use, etc) spec-feature This is a request to implement a new feature which is already specified by the OTel specification spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug and removed triage spec-feature This is a request to implement a new feature which is already specified by the OTel specification labels Mar 26, 2025
cjihrig added a commit to cjihrig/opentelemetry-js that referenced this issue Apr 20, 2025
… input validation

This commit updates the PeriodicExportingMetricReader() input
validation to properly handle default and explicitly provided
timeout and interval values.

The validation is also refactored to use destructuring, as this
makes the code more robust against things like maliciously
crafted getters. It arguably makes the code more readable as well.

Fixes: open-telemetry#5550
cjihrig added a commit to cjihrig/opentelemetry-js that referenced this issue Apr 20, 2025
… input validation

This commit updates the PeriodicExportingMetricReader() input
validation to properly handle default and explicitly provided
timeout and interval values.

The validation is also refactored to use destructuring, as this
makes the code more robust against things like maliciously
crafted getters. It arguably makes the code more readable as well.

Fixes: open-telemetry#5550
@pichlermarc pichlermarc removed the spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug label May 12, 2025
@pichlermarc pichlermarc reopened this May 12, 2025
@pichlermarc
Copy link
Member

Reopened this as the fix for it might've broken a common case where the interval is set but the timeout is not. We may have to look into alternative ways to do this, see #5671 (review)

cjihrig added a commit to cjihrig/opentelemetry-js that referenced this issue Jun 2, 2025
… input validation

This commit is another take on open-telemetry#5621. That PR needed to be reverted because
it could break existing code that implicitly set PeriodicExportingMetricReader
timeout and interval values.

This time around, instead of throwing, bad implicit values are clamped as
suggested in open-telemetry#5671.

Fixes: open-telemetry#5550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:sdk-metrics priority:p3 Bugs which cause problems in user apps not related to correctness (performance, memory use, etc)
Projects
None yet
2 participants