ExecutorServiceMetrics should allow customization of metric name

[anuraaga]

Unlike many JVM metrics, ExecutorService is actually instantiated by users and there may be many in an app. If the metric id can't be customised, multiple executors with different tags will fail to register due to Prometheus restrictions.

Is it ok to add an ID prefix to the factories for executor metrics? Happy to make the change if so but one thing that comes to mind is given there are already so many factories, if every one had a prefix version it would be a lot of new methods, wonder what a good practice in this codebase would be for that.

More context in line/armeria#2605

[jkschneider]

@anuraaga I believe this is in a way already supported, since Micrometer is adding a name tag to the all executor service metrics based on a name provided to either the static builder or constructor:

• Executor monitor(MeterRegistry registry, Executor executor, String executorName, Iterable<Tag> tags)
• ExecutorServiceMetrics(@Nullable ExecutorService executorService, String executorServiceName, Iterable<Tag> tags)

Having this be a tag allows you to reason about all ExecutorService in your app or dimensionally drill down on a named one.

Nevertheless, if you prefer to split these metrics out as individual metric names rather than differentiating ExecutorService by the name tag, then you should be able to accomplish it with a MeterFilter:

new MeterFilter() {
    @Override
    public Meter.Id map(Meter.Id id) {
        if(id.getName().startsWith("executor")) {
            // this generates metrics like "myexecutor.executor.queued", "myexecutor.executor.active"
            return id.withName(id.getTag("name") + "." + id.getName())
                    .withTags(...); // you could go further to strip the name tag if you want
        }
        return id;
    }
}

[jkschneider]

Perhaps I don't fully understand the issue though. Is the issue that different libraries (e.g. Armeria and Reactor) could each choose to instrument different ExecutorService with the same executor service name?

/cc @trustin

[anuraaga]

Yes since ExecutorService can be created for various reasons, it's quite normal for there to be many from various libraries or even user code in the app itself.

The problem isn't about visibility per se as the name tag indeed allows differentiating metrics as you say. But the constructor for ExecutorServiceMetrics accepts a custom list of tags. This means a library may add their own tag (reactor does this) and the app will crash with IllegalStateException if it is used along another ExecutorServiceMetrics because Prometheus does now allow two metrics with the same name "executor" to have a different set of tags.

The meter filter can work but it seems like a tedious workaround to solve a naming clash that causes the app to crash.

I would say because of Prometheus's restrictions there is a general restriction that any metric that can be initialized by a user must allow customization of name if it allows customization of tags to avoid the IllegalStateException for same-name, different tags. Does that make sense?

[anuraaga]

Sorry accidentally hit close - lately do that a lot on phone :(