Skip to content

Conversation

bryce-b
Copy link
Member

@bryce-b bryce-b commented May 9, 2025

This PR will fix the issue of metric builders needing to be cast to access certain methods.

Prior to this change:

    let meterProvider = StableMeterProviderBuilder()
      .registerMetricReader(reader: myReader)
      .build()
    let meter = meterProvider.meterBuilder(name: "meter").build() as! StableMeterSdk
    let instrument = (meter.upDownCounterBuilder(name: "updown") as! LongUpDownCounterBuilderSdk)
      .setUnit("unit")
      .setDescription("description")
      .build() as! LongUpDownCounterSdk

after

let meterProvider = StableMeterProviderBuilder()
      .registerMetricReader(reader: myReader)
      .build()
    let meter = meterProvider.meterBuilder(name: "meter").build()
    let instrument = meter.upDownCounterBuilder(name: "updown")
      .setUnit("unit")
      .setDescription("description")
      .build()

One issue I have yet to solve is how to deal with the StableMeterProviderSdk. It's not possible to return the NOOP provider when no reader is registered.

  public func meterBuilder(name: String) -> MeterBuilderSdk {
//    if registeredReaders.isEmpty {
//      return DefaultStableMeterProvider.noop()
//    }

@bryce-b bryce-b force-pushed the metrics-type-refactor branch from 70daded to 072f41d Compare May 9, 2025 20:54
Copy link
Contributor

@martin308 martin308 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me. One question about a block of commented out code

@bryce-b bryce-b marked this pull request as ready for review May 28, 2025 23:07
@bryce-b
Copy link
Member Author

bryce-b commented May 28, 2025

I was able to resolve the issue by adding the NoopStableMeterProviderBuilder, which is the default builder, and it will return a StableMeterProviderBuilder from its registerMetricReader builder method.

@bryce-b bryce-b merged commit 400664f into main Jun 3, 2025
9 checks passed
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.

4 participants