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

[sdk-metrics] Metric Instruments are not available in MetricReaders added after creation #4112

Open
hectorhdzg opened this issue Sep 1, 2023 · 5 comments
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect

Comments

@hectorhdzg
Copy link
Member

What happened?

Steps to Reproduce

Create some Metric instruments in a specific Meter, then create another Metric reader exporting the data somewhere else.

Expected Result

All Metric instruments are available in all Metric readers

Actual Result

Only Metrics instruments created while the readers were present are available

Additional Details

This is a simplistic code to demonstrate the issue, the actual problem we are having is that the Metric Instruments are created from OTel Instrumentation libraries in Azure distro package, and we will like to export those using a different exporter.

OpenTelemetry Setup Code

import { OTLPMetricExporter } from "@opentelemetry/exporter-metrics-otlp-http";
import {
  ConsoleMetricExporter,
  MeterProvider,
  PeriodicExportingMetricReader,
} from "@opentelemetry/sdk-metrics";

const meterProvider = new MeterProvider();
// Add OTLP Exporter
const otlpMetricsExporter = new OTLPMetricExporter();
const otlpMetricReader = new PeriodicExportingMetricReader({
  exporter: otlpMetricsExporter,
});
meterProvider.addMetricReader(otlpMetricReader);
// Create metric
const testCounter1 = meterProvider.getMeter("testMeter").createCounter("testCounter1");

// Add console Exporter
const metricReader = new PeriodicExportingMetricReader({
  exporter: new ConsoleMetricExporter()
});
meterProvider.addMetricReader(metricReader);
// Create another metric
const testCounter2 = meterProvider.getMeter("testMeter").createCounter("testCounter2");

setInterval(() => {
  // Increase counters
  testCounter1.add(1);
  testCounter2.add(2);
}, 10000);

package.json

No response

Relevant log output

No response

@hectorhdzg hectorhdzg added bug Something isn't working triage labels Sep 1, 2023
@hectorhdzg
Copy link
Member Author

Forgot to add the output, only testCounter2 is available in console

{
descriptor: {
name: 'testCounter2',
type: 'COUNTER',
description: '',
unit: '',
valueType: 1
},
dataPointType: 3,
dataPoints: [
{ attributes: {}, startTime: [Array], endTime: [Array], value: 10 }
]
}

@pichlermarc
Copy link
Member

Hmm, I think our metrics SDK is not built in a way to handle this case. IIRC none of the other language implementations support registering readers after creating a MeterProvider.

@hectorhdzg In your specific case, would it be possible to move the second call to addMetricReader() to before the instruments are created? 🤔

I think offering this kind of after-the-fact registration was not optimal and something we should consider dropping in SDK 2.0. Maybe we should only allow to add readers via MeterProvider constructor options. (We did move view registration to the constructor for the very same reason #3066)

@arendjr
Copy link

arendjr commented Sep 5, 2023

I just ran into this same issue today and it creates for some really unintuitive and hard-to-debug behavior in the Otel library. Do note in our case we do create the MeterProvider first, it's merely the registration of the MetricReader that's postponed. For us, we cannot ensure the call to addMetricReader() happens before the creation of counters, because the meter provider and the readers live in separate packages. Even if we could, frankly I don't think we want to, since we want to support dynamic situations where a client may perform an async call before the reader is registered, since we're creating a reusable library and we cannot enforce the order in which consumers call certain functions.

For an example of the type of trickery people run into when trying to enforce calling order, see this issue filed against our project: autometrics-dev/autometrics-ts#113
In short: code in decorators always gets invoked before any other code in a module, and since our decorators register metrics, things get called sooner than one may expect. Similar issues might occur when someone imports code from another module which happens to try to create metrics. Altogether, I don't think it's reasonable to expect the metric reader to always be the first thing to be instantiated in a project.

@david-luna
Copy link
Contributor

@pichlermarc you pointed out that #4419 fixes this issue. I've tried with the latest version and indeed it works.

IIUC the deprecation of addMetricReader and future removal of it in next major will compel users to define metric readers upfront (in MeterProvider constructor) which will make the approach commented in last comment not viable.

I'm not opposing to that change but I guess we should highlight this and provide some guidance so users (like autometrics-dev) can adapt their codebase.

@pichlermarc
Copy link
Member

@pichlermarc you pointed out that #4419 fixes this issue. I've tried with the latest version and indeed it works.

IIUC the deprecation of addMetricReader and future removal of it in next major will compel users to define metric readers upfront (in MeterProvider constructor) which will make the approach commented in last comment not viable.

I'm not opposing to that change but I guess we should highlight this and provide some guidance so users (like autometrics-dev) can adapt their codebase.

@david-luna IIUC this issue would really be fixed by implementing a delegating no-op for metrics as defined in #3622. 🤔

As all data-points written to the instruments would be dropped anyway in absence of a MetricReader. By implementing a feature that allows for registering a MeterProvider + all MetricReaders at a later time (as is currently the case with TracerProviders), we would eliminate the need of a MetricReader that's registered in a delayed manner. WDYT? 🤔

This would also mean that this issue here is a duplicate of #3622

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

No branches or pull requests

4 participants