micrometer icon indicating copy to clipboard operation
micrometer copied to clipboard

Possible performance issues with usage of ....builder

Open Hronom opened this issue 5 years ago • 6 comments

We observing that solution like this:

            return Timer
                    .builder("execution.time")
                    .tag("executionName", name)
                    .tag("executionFullName", fullName)
                    .publishPercentiles(MetricsConstraints.defaultTimerPercentiles)
                    .register(Metrics.globalRegistry)

is slow compare to:

        return timersByFullName.computeIfAbsent(fullName, s -> Timer
                .builder("execution.time")
                .tag("executionName", name)
                .tag("executionFullName", fullName)
                .publishPercentiles(MetricsConstraints.defaultTimerPercentiles)
                .register(Metrics.globalRegistry));

In second solution we use private static final ConcurrentHashMap<String, Timer> timersByFullName = new ConcurrentHashMap<>();.

Why such performance degradation happens in micrometer? Is it because we have a lot of metrics in Metrics.globalRegistry so it's becomes slow compare to local map timersByFullName.

What is the advice you can provide here? Will first solution win, if we will use instead of Metrics.globalRegistry - local registry for this metrics and add it in Metrics.globalRegistry.add(localMeterRegistry);

Thanks!

Hronom avatar Sep 28 '20 14:09 Hronom

Can you provide some proof to your findings? Do you have any benchmarks?

marcingrzejszczak avatar Dec 20 '23 12:12 marcingrzejszczak

Why such performance degradation happens in micrometer?

There is a cost to the builder but it provides the most flexibility and usability. In your alternative, you need to compute a correct key that uniquely identifies each Timer. I don't know whether you are doing that given the code snippet, but note that a meter is uniquely identified by its name and full set of tags, after any registered meter filters are applied. We of course want to keep the overhead of using the builder as low as possible, but we need to keep usability. And for most users, the cost should be negligible, but of course it depends on the use case. We're open to suggestions and are always striving to improve the performance of Micrometer. Let us know if you've identified something we can improve.

shakuzen avatar Dec 21 '23 10:12 shakuzen

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

github-actions[bot] avatar Jan 02 '24 10:01 github-actions[bot]

Could you please check if this feature could be useful for you: https://github.com/micrometer-metrics/micrometer/pull/4097?

jonatan-ivanov avatar Jan 08 '24 12:01 jonatan-ivanov

I understand that there is a performance overhead associated with creating meters. See more info in this repository where I have tried to benchmark the results.

In ideal scenario (no meterfilters), the creation cost is cheaper. But, I bet every practical use of micrometer involves multiple meterfilters (the one's which overrides map()) to rename metrics, add tags, validate tags etc. In this scenario, it is becoming costly to construct a new meter.Id and go through all the meter filter's to create additional meter.Id's at each stage and ultimately the registry would contain that meter and we will basically discard everything we have done until now.

E.g: Assume 2 meter filters are registered, 1 - add's a common tags (service=xyz), 2 - add's prefix("abc.") to a meter. When trying to increment a request counter request{key=value}, it goes through the first meterfilter and creates a new Meter.Id with request{key=value,service=xyz} and then to the next filter again a new Meter.Id abc.request{key=value, service=xyz}. Paying this one-time cost is acceptable in most cases.

But when next request for same combinatiuon comes in, we have to repeat the entire step and at the end getOrCreateMeter will discard this new object and return the already registered meter.

This is one of the basic scenarios I have described here, but usually for a single HTTP request there would be multiple meters created for measuring different things and everything has to go this route making this a noticeable overhead. We are using a ConcurrentHashMap to reduce this overhead and we have seen significant performance gains with that, but all these are scattered all over the codebase and it would be good to provide a solution that can adopted by users for these use-cases. As mentioned in this thread, adding meterfilters basically invalidates the cache and these can be managed easily in a centralized cache specific to a registry.

lenin-jaganathan avatar Jan 09 '24 08:01 lenin-jaganathan

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

github-actions[bot] avatar Jan 17 '24 01:01 github-actions[bot]