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

Micrometer integration improvements #1476

Closed
felixbarny opened this issue Nov 6, 2020 · 3 comments
Closed

Micrometer integration improvements #1476

felixbarny opened this issue Nov 6, 2020 · 3 comments
Assignees
Labels
agent-java enhancement Enhancement of an existing feature
Milestone

Comments

@felixbarny
Copy link
Member

felixbarny commented Nov 6, 2020

Currently, there are some pitfalls when using the Micrometer integration.

There are two main ways to configure Micrometer MeterRegistries: CountingMode.CUMULATIVE, representing monotonically increasing counters and CountingMode.STEP, that reset the counters.

We're currently recommending CountingMode.STEP so that we report the deltas since the last report which simplifies common aggregations such as summing counters across different instances. However, this requires that the metrics_interval is lined up with the step duration. Even when doing that, the time when we read the metrics would need to be aligned with the time they get reset which is not really feasible. With some unlucky timing, we might always report the value just after it has been reset.

Update: I read the code wrong: it always returns the value from the last interval so that shouldn't be an issue. For example, if the step duration is 10s, it would return the count from the first 10s when querying the value at t=15s

When there are multiple MeterRegistrys within a CompoundMeterRegistry, we de-duplicate the metrics by putting them in a map that has the Meter.Id as a key. However, we don't consistently favor STEP or cumulative CUMULATIVE, or a specific type of MeterRegistry.

final Map<Meter.Id, Meter> meters = new HashMap<>();
@Override
public void accept(Meter meter) {
Meter.Id meterId = meter.getId();
if (WildcardMatcher.isNoneMatch(disabledMetrics, meterId.getName())) {
meters.put(meterId, meter);
}
}

We don't support histograms yet but there are also some open questions to investigate: For the SimpleMeterRegistry it seems no matter whether choosing STEP or CUMULATIVE, the histogram gets reset based on io.micrometer.core.instrument.distribution.DistributionStatisticConfig#getExpiry. However, for the PrometheusMeterRegistry for example, the histogram never gets reset (as the expiry is set to 5 years).

Also, I'm not sure if percentile aggregation on a histogram field would even work if the histogram is tracking incrementing counts.

cc @cyrille-leclerc @alex-fedotyev

@felixbarny felixbarny added this to the 7.11 milestone Nov 6, 2020
@felixbarny
Copy link
Member Author

Marking Micrometer support as beta for now: #1492

@henrikno
Copy link

I was surprised when I saw that micrometer itself did the diff calculations. I expected the recording to use e.g. monotonic counters and then the reporter to do the diff calculations, that way they're synchronized with the collection interval, and you could have multiple reporters with different intervals, etc.
When collecting prometheus metrics with metricbeat it has the option of pre-calculating the diff for you with rate_counters https://www.elastic.co/guide/en/beats/metricbeat/current/metricbeat-metricset-prometheus-collector.html
Personally I think it's nice to have both (at least as an option), because one is easier to visualize and sort by, but the other is more accurate.
Do you think it's possible to do something similar in the APM agent?

@felixbarny
Copy link
Member Author

I was surprised when I saw that micrometer itself did the diff calculations.

It only does that if you're configuring Micrometer with CountingMode.STEP. When configuring it with CountingMode.CUMULATIVE, it monotonically increments the counter.

Do you think it's possible to do something similar in the APM agent?

For the Prometheus client integration (elastic/apm#355), we want to test out calculating diffs at the agent level, similar to rate_counters in Metricbeat. That's likely going to be the default and potentially there could be an option to send incrementing counters instead.

For Micrometer, I think the best way to configure delta vs incrementing counters is to use the Micrometer settings (STEP vs CUMULATIVE).

@SylvainJuge SylvainJuge modified the milestones: 7.11, 7.12 Jan 4, 2021
@AlexanderWert AlexanderWert removed this from the 7.12 milestone Jan 29, 2021
@AlexanderWert AlexanderWert added this to the 7.15 milestone Jun 28, 2021
@AlexanderWert AlexanderWert modified the milestones: 7.15, 7.16 Aug 24, 2021
@SylvainJuge SylvainJuge added the enhancement Enhancement of an existing feature label Aug 30, 2021
@AlexanderWert AlexanderWert added this to the 8.4 milestone Jul 25, 2022
@AlexanderWert AlexanderWert modified the milestones: 8.4, 8.5 Jul 25, 2022
@AlexanderWert AlexanderWert modified the milestones: 8.5, 8.6 Sep 28, 2022
@AlexanderWert AlexanderWert modified the milestones: 8.6, 8.7 Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java enhancement Enhancement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants