Skip to content
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

Proposal: Clear InMemoryMetricExporter Data Post-Shutdown to Align with Other Exporters #5131

Open
1 of 2 tasks
paper2 opened this issue Nov 10, 2024 · 1 comment · May be fixed by #5214
Open
1 of 2 tasks

Proposal: Clear InMemoryMetricExporter Data Post-Shutdown to Align with Other Exporters #5131

paper2 opened this issue Nov 10, 2024 · 1 comment · May be fixed by #5214

Comments

@paper2
Copy link
Contributor

paper2 commented Nov 10, 2024

  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first

Proposal

The InMemoryMetricExporter currently allows users to retrieve metrics through getMetrics even after the shutdown method has been called. To improve consistency with other in-memory exporters (such as InMemorySpanExporter and InMemoryLogRecordExporter), it may be beneficial to clear the metrics array after shutdown, preventing further retrieval.

If this proposal is accepted, I would be interested in implementing the changes myself :)

Background

Metric Exporter Specification

According to the specification, allowing getMetrics to return values after shutdown is technically compliant.

Shuts down the exporter. Called when SDK is shut down. This is an opportunity for exporter to do any cleanup required.

Shutdown SHOULD be called only once for each MetricExporter instance. After the call to Shutdown subsequent calls to Export are not allowed and should return a Failure result.

Shutdown SHOULD NOT block indefinitely (e.g., if it attempts to flush the data and the destination is unavailable). OpenTelemetry SDK authors MAY decide if they want to make the shutdown timeout configurable.

However, based on the line:

This is an opportunity for exporter to do any cleanup required.

it seems more intuitive that the getMetrics function should return a cleared array after shutdown, in line with expected behavior for cleanup.

Current Implementation of InMemoryMetricExporter

Currently, InMemoryMetricExporter does not clear the array returned by getMetrics at shutdown. As a result, users can still retrieve data with getMetrics even after shutdown.

shutdown(): Promise<void> {
this._shutdown = true;
return Promise.resolve();
}

Implementation in InMemorySpanExporter and InMemoryLogRecordExporter

In contrast, InMemorySpanExporter does clean up its data after shutdown, clearing the array returned by getFinishedSpans.

shutdown(): Promise<void> {
this._stopped = true;
this._finishedSpans = [];
return this.forceFlush();
}

Similarly, InMemoryLogRecordExporter also resets its internal state at shutdown.

public shutdown(): Promise<void> {
this._stopped = true;
this.reset();
return Promise.resolve();
}

This inconsistency in behavior among different in-memory exporters could lead to confusion for users (as it did for me).

@dyladan
Copy link
Member

dyladan commented Nov 14, 2024

I don't think the difference here is intentional

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 a pull request may close this issue.

2 participants