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

Can't configure DISTRIBUTION on per-metric basis for Datadog #2798

Closed
hdv opened this issue Sep 29, 2021 · 0 comments · Fixed by #2799
Closed

Can't configure DISTRIBUTION on per-metric basis for Datadog #2798

hdv opened this issue Sep 29, 2021 · 0 comments · Fixed by #2799
Labels
bug A general bug registry: statsd A StatsD Registry related issue
Milestone

Comments

@hdv
Copy link
Contributor

hdv commented Sep 29, 2021

Describe the bug
publishPercentileHistogram for all Timers/Summaries is set by the very first Timer/Summary and publishPercentileHistogram flag in timers/summaries that are created after that is ignored.

Environment

  • Micrometer version [1.8.0-M3]
  • Micrometer registry [StatsD with Datadog flavor]
  • OS: [macOS]
  • Java version: ❯ java --version
    openjdk 11.0.2 2019-01-15
    OpenJDK Runtime Environment 18.9 (build 11.0.2+9)
    OpenJDK 64-Bit Server VM 18.9 (build 11.0.2+9, mixed mode)

To Reproduce
How to reproduce the bug:

    @Test
    void timerSentAsDatadogDistribution_whenPercentileHistogramDisabled() {
        final Sinks.Many<String> lines = sink();
        final StatsdConfig config = configWithFlavor(StatsdFlavor.DATADOG);
        registry = StatsdMeterRegistry.builder(config)
                .clock(clock)
                .lineSink(toLineSink(lines, 2))
                .build();

        StepVerifier.create(lines.asFlux())
                .then(() -> {
                    Timer.builder("my.timer2").publishPercentileHistogram(false).register(registry).record(20, TimeUnit.SECONDS);
                    Timer.builder("my.timer").publishPercentileHistogram(true).register(registry).record(2, TimeUnit.SECONDS);
                })
                .expectNext("my.timer2:20000|ms")
                .expectNext("my.timer:2000|d")
                .verifyComplete();
    }

    @Test
    void timerSentAsDatadogDistribution_whenPercentileHistogramEnabled() {
        final Sinks.Many<String> lines = sink();
        final StatsdConfig config = configWithFlavor(StatsdFlavor.DATADOG);
        registry = StatsdMeterRegistry.builder(config)
                .clock(clock)
                .lineSink(toLineSink(lines, 2))
                .build();

        StepVerifier.create(lines.asFlux())
                .then(() -> {
                    Timer.builder("my.timer").publishPercentileHistogram(true).register(registry).record(2, TimeUnit.SECONDS);
                    Timer.builder("my.timer2").publishPercentileHistogram(false).register(registry).record(20, TimeUnit.SECONDS);
                })
                .expectNext("my.timer:2000|d")
                .expectNext("my.timer2:20000|ms")
                .verifyComplete();
    }

Expected behavior
See the failing tests above.

Additional context
Issue was introduced with this PR: https://github.com/micrometer-metrics/micrometer/pull/2745/files

Problematic change seems to be introduction of the second `StatsdLineBuilder lineBuilder(Meter.Id id, @nullable DistributionStatisticConfig distributionStatisticConfig)' method and the fact that initial overload without DSC parameter was kept in place and being used.

The second problem that builds on top of the first one is the fact that lineBuilderFunction is cached in StatsdMeterRegistry.lineBuilder(Meter.Id, DSC) method when it must be created every single time now as we must be able to configure 2 different DatadogStatsdLineBuilder's - one that generates percentileHistogram and reports as DISTRIBUTION and another one that reports as regular TIMER or HISTOGRAM.

@hdv hdv added the bug A general bug label Sep 29, 2021
@jonatan-ivanov jonatan-ivanov changed the title [1.8.0-M3][StatsD.Datadog] Can't configure DISTRIBUTION on per-metric basis Can't configure DISTRIBUTION on per-metric basis for Datadog Oct 1, 2021
@jonatan-ivanov jonatan-ivanov added this to the 1.8.0-RC1 milestone Oct 1, 2021
@jonatan-ivanov jonatan-ivanov added the registry: statsd A StatsD Registry related issue label Oct 1, 2021
jonatan-ivanov pushed a commit that referenced this issue Oct 1, 2021
* Ensure distribution can be configured per-metric

Fix #2798

* Not everything is a timer

* Ensure backward compatibility
izeye added a commit to izeye/micrometer that referenced this issue Oct 18, 2021
shakuzen pushed a commit that referenced this issue Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug registry: statsd A StatsD Registry related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants