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

Attach openTelemetry metrics to zio.metric.Histogram #423

Open
lgajowy opened this issue Jul 1, 2022 · 1 comment
Open

Attach openTelemetry metrics to zio.metric.Histogram #423

lgajowy opened this issue Jul 1, 2022 · 1 comment
Assignees

Comments

@lgajowy
Copy link
Contributor

lgajowy commented Jul 1, 2022

Proposal Link with code snippets on how to do that (see Part 2): here

ZIO defines Histograms internally (eg fibersStarted up/down counter). Similarily to Gauges and Counters we should collect histogram data. The caveat is that there is no async version of a histogram in opentelemetry yet, so periodic collection has to be implemented too (see proposal).

Goals:

  • propagating counter data to otel

NON-goals:

  • excluding particular metrics collection. This should be planned and done in a separate issue
  • adding a sophisticated configuration. That can be postponed.
@lgajowy
Copy link
Contributor Author

lgajowy commented Jul 19, 2022

What's described in the proposal won't work due to the following:

  • if we decide to "mimic" an async histogram behavior:
val periodicDataCollection = for {
     value <- histogram.value
     _ = value.buckets.foreach {
       // TODO: So if a bucket will contain  1_000_000 values that means it will be called 1 000 000 times. Unacceptable.
       case (bucket, countPerBucket) => (1 to countPerBucket) foreach { _ =>
         otelHistogram.record(bucket)
       }
     }
   } yield ()

It doesn't make sense due to the fact we will need to call .record countPerBucket times

  • the proposal didn't take histogram.boundaries into account. In openTelemetry you can modify the boundaries in a separate extension during sdk initialization: (link)
import com.google.auto.service.AutoService;
import io.opentelemetry.sdk.autoconfigure.spi.AutoConfigurationCustomizer;
import io.opentelemetry.sdk.autoconfigure.spi.AutoConfigurationCustomizerProvider;
import io.opentelemetry.sdk.metrics.Aggregation;
import io.opentelemetry.sdk.metrics.InstrumentSelector;
import io.opentelemetry.sdk.metrics.View;

import java.util.Arrays;

@AutoService(AutoConfigurationCustomizerProvider.class)
public class HistogramConfigurer implements AutoConfigurationCustomizerProvider {

    @Override
    public void customize(AutoConfigurationCustomizer autoConfiguration) {
        System.err.println("*** HistogramConfigurer.customize() is called ***");
        autoConfiguration.addMeterProviderCustomizer((sdkMeterProviderBuilder, configProperties) -> sdkMeterProviderBuilder.registerView(
                InstrumentSelector.builder()
                        .setMeterName("http.server.duration")
                        .build(),
                View.builder()
                        .setAggregation(Aggregation.explicitBucketHistogram(Arrays.asList(1.0, 2.0, 3.0)))
                        .build()));
    }
}

If we want to instrument the Metric.histogram() method, we don't have a way of passing the histogram.Boundaries to the HistogramConfigurer (it will be already invoked). That problem could be mitigated with passing a proper config during agent startup that would allow users defining the boundaries for otel instruments. This however is not good user experience.

I'm currently looking for ways of attaching the synchronous histogram not to the histogram constructor but to the update function (otel's "record" equivalent).

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

No branches or pull requests

1 participant