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

Meter metrics get override in instrumentations after setMeterProvider has called #3249

Closed
1 of 2 tasks
osherv opened this issue Sep 13, 2022 · 5 comments · Fixed by #3267
Closed
1 of 2 tasks

Meter metrics get override in instrumentations after setMeterProvider has called #3249

osherv opened this issue Sep 13, 2022 · 5 comments · Fixed by #3267
Assignees
Labels
question User is asking a question not related to a new feature or bug signal:metrics Issues and PRs related to general metrics signal

Comments

@osherv
Copy link
Member

osherv commented Sep 13, 2022

Hey guys, I'm currently working on the MongoDB metrics collections and noticed some issue (not huge but can lead to many bugs in the future).
In order to produce metrics, we need to create some metric instruments, and these metric instruments automatically get overridden when setting a new meter provider.
Maybe it's ok and that's the best practice but it requires the instrumentations to implement something like:

export class MongoDBInstrumentation extends InstrumentationBase {
  private _connectionsUsage!: UpDownCounter;

  constructor(protected override _config: MongoDBInstrumentationConfig = {}) {
    super('@opentelemetry/instrumentation-mongodb', VERSION, _config);
    this._updateMetricInstruments();
  }

  override setMeterProvider(meterProvider: MeterProvider) {
    super.setMeterProvider(meterProvider);
    this._updateMetricInstruments();
  }

  private _updateMetricInstruments() {
    this._connectionsUsage = this.meter.createUpDownCounter(
      'active_connections',
      {
        description:
          'The number of connections that are currently in state described by the state attribute.',
      }
    );
  }
  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first
@pichlermarc pichlermarc added the signal:metrics Issues and PRs related to general metrics signal label Sep 14, 2022
@pichlermarc
Copy link
Member

Hmm, I wonder if we should add an optional updateMetricInstruments() to InstrumentationAbstract that is called when setMeterProvider() is called. I feel like I don't know enough about the instrumentations to give a definitive answer though. 🤔

@pichlermarc pichlermarc added the question User is asking a question not related to a new feature or bug label Sep 14, 2022
@legendecas
Copy link
Member

I can see this common metrics problem diverging from the tracing part as the metric instruments need to be re-created after the meter provider has been updated. An abstract method called createMetricInstruments or updateMetricInstruments sounds good to me.

@osherv
Copy link
Member Author

osherv commented Sep 15, 2022

Yea sounds like a good solution. You can assign this to me :)

@weyert
Copy link
Contributor

weyert commented Sep 20, 2022

How will this work when you want to use histogram as a metric? You would need to define a view for a histogram to ensure the correct buckets are used and you can only define Views when instantiating the meter provider.

@legendecas
Copy link
Member

@weyert: How will this work when you want to use histogram as a metric? You would need to define a view for a histogram to ensure the correct buckets are used and you can only define Views when instantiating the meter provider.

Instrumentations are supposed to create histogram metrics in the _updateMetricInstruments method. I don't think there is any difference between histograms with other types of metrics.

Yes, a MeterProvider must be constructed with views before setting it up with instrumentation. That is to say, metrics API didn't provide a means to instruct the SDK to configure the buckets for a histogram right now. It's being tracked at open-telemetry/opentelemetry-specification#2229.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question User is asking a question not related to a new feature or bug signal:metrics Issues and PRs related to general metrics signal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants