Get rid of cases where API code does implementation's work

[jmartisk]

We're doing way too much stuff in the API code that should be done by implementations instead. We might want to investigate how to minimize this while not disrupting existing implementations (or even user applications) too much.

[jmartisk]

One specific thing that should be resolved is the MetricID constructor, which, even though it is called quite often, has a relatively high performance cost because it calls ConfigProvider.getConfig(). We should open a way for implementation to supply the tags by itself, in which case it will be able to cache them and not retrieve Config every time.

[loicmathieu]

Any news on this ? Real application profiling shows 10% global memory allocation due to MetricID.<init>.

[jmartisk]

This would be a breaking change, we're currently discussing whether the next release will be a breaking release. For that, we need more breaking changes to make it worth doing a major version bump.

In the meanwhile, feel free to suggest how exactly this should be done :) I've been thinking of requiring a class that supplies the global tags, and that class would be referenced by the ServiceLoader mechanism. I'm not sure if that can cover all use cases though, we need to think what arguments the tag supplier needs to be able to be able to determine the correct tags. We should also consider application server uses where there are multiple applications and the supplier needs to know which application called it, so it can supply the correct _app tag. One more potential problem is that ServiceLoader mechanisms are not integrated with CDI, so we need to make sure the lifecycle of the tag supplier will not be an issue, especially in multi-app environments.

[loicmathieu]

@jmartisk the issue I have on MetricID can be workarouned in two ways :

• Loading the global tags once (but I don't know if it's possible as I don't know if configuration can be changed at rutime ... for Quarkus this is not the case but at the spec level I don't know). Maybe this can be implemented via a flag that turn this functionality on/off.
• Adding a constructor or a factory Method that allow to give the global tags at construction time so the call site can cache them (this means that smallrye-metrics should use this flag and the issue with runtime configuration should be handle there). new MetricID(String name, List<Tag> globalTags, Tags... tags)

[aseovic]

We actually just ran into this issue as well (actually, there are multiple issues with the current MetricID implementation, and global tags support in general):

1. It is inefficient, as you pointed out above

Not only does it do a config lookup, but it also parses global tags every single time, and to make things worse doesn't even use a compiled regex to do so.

2. It is not flexible enough

The assumption that only end users could provide global tags for metrics, and that those tags can only be provided via config, is simply wrong.

We need to ensure that we add environment-related tags to all the metrics created within each application process (not only our own metrics, but also base metrics, other vendor metrics and any application metrics end users may publish) in order to make them unique across the cluster, and we need to do so programmatically, not via config.

I do believe that a ServiceLoader based approach is the right thing to do here, as it allows anyone to contribute the tags to a set of global tags that can then be applied to all the metrics. It can also easily support current global tags implementation, as implementing ConfigTagsProvider that does the same thing MetricID constructor currently does (once, instead of every time MetricID is constructed) is trivial.

I also think loading global tags once (at startup) and caching them is the right thing to do. Changing a set of global tags at runtime introduces all kinds of issues, as it effectively ends up creating new metrics, which can be problematic. I do think though that the spec should explicitly define this behavior, so the users can rely on it, instead of leaving it up to the implementations to decide what they want to do.

3. It is not implemented with extensibility in mind

If there was at least a protected constructor that simply accepts name and tags and sets corresponding fields, it would allow implementations to extend MetricID class and override (and optimize) the default behavior. Making some of the helper methods protected instead of private would also help.

While my personal preference is to add an official, ServiceLoader-based approach to define global tags to the spec, making MetricID extensible would allow MP implementations to override default behavior without breaking compatibility for those who don't, so it may be a decent interim solution.

[jmartisk]

I think we could go with the ServiceLoader approach. There would be a GlobalTagsProvider interface, and an implementation of it will be expected to be obtainable via ServiceLoader. That implementation would be called from the MetricID constructor. (Maybe someone more knowledgeable will object that retrieving the service instance each time is still a performance issue, I don't know)

The interface would be like

// Retrieves global tags that should be appended to each metric. Can return null.
Tag[] getGlobalTags();

I'm thinking that we could also reuse that interface for obtaining the application name

// value for the "_app" tag, or null if no such tag should be added
String getApplicationName();

Even though some people might argue this is out of scope of MicroProfile. We already do have a mechanism for obtaining the appname, so we should not drop it, but rather change it in line with the global tags change. The impl should be able to detect which application this is by either looking at the TCCL, or scanning the stack trace. If someone has an idea what else we could do for the impl to facilitate this (meaning what arguments the getApplicationName method should have), that'd be welcome.

Looking at this, if we introduced such interface with these two methods, it seems we would eliminate the MP Config compile-time dependency from Metrics. Implementations would probably still need it, but the spec API would not.

What do you guys think, is this suitable?

[loicmathieu]

As long as implementors (and I hope smallrye will take advantage of it as soon as possible) will be able to create a MetricID without accessing the ConfigProvider I'm OK with it. I trust you to find the best way to do this ;)

[jmartisk]

If we add to the spec that the mp.metrics.tags value must be immutable at runtime (or that the implementation is allowed to decide to consider it immutable), then the implementation can safely access the ConfigProvider just once and then cache the value. And I do think the spec should say that, because otherwise we would basically force the implementations to check the ConfigProvider again each time, so this change would be quite useless.

[aseovic]

@jmartisk I like the proposal, with a few minor changes:

1. Introduce a MetricID constructor that takes metric name and a Map of tags as arguments (to differentiate it from the existing one that takes an array of tags) and does nothing but set the corresponding fields. That would allow implementations to optimize tag construction any way they see fit. Better yet, introduce a constructor that takes global tags and metric-level tags as separate arguments, as @loicmathieu suggested earlier, in which case they can both be either an array or a map (I'm leaning towards map, as that's what they are ultimately stored in internally, but either will work).

In any case, whatever the type of globalTags constructor argument, that should be what's returned by the GlobalTagsProvider.getGlobalTags method, so there is no conversion there.

2. Do we really need to treat _app tag as special? It is easy enough to implement GlobalTagsProvider for it in the environments that need/want to define it.

[aseovic]

Actually, for 1 above we could actually pass GlobalTagsProvider as a second argument instead, which would allow implementations to always pass the same instance, which could easily cache the ServiceLoader lookup results if so desired.

The default/current constructor could then be changed to use the default implementation, which would simply do a ServiceLoader lookup and aggregate global metrics returned by all the discovered providers.

And yes, the spec should say that all global tags, not only the ones defined by the mp.metrics.tags config property, are initialized once at startup and cannot be modified afterwards.

I'm happy to submit a PR for this, btw, if you want me to.

[donbourne]

Introduce a MetricID constructor that takes metric name and a Map of tags as arguments (to differentiate it from the existing one that takes an array of tags) and does nothing but set the corresponding fields.

I think what you're aiming to achieve here is to let the runtime create a MetricId with whatever global tags it needs. I agree that that's a good thing (to avoid a hardcoded impl of MetricId tag lookup that has performance issues), but I'm not sure the programming model should introduce the ability to set global settings, which should be invariant, with each new MetricId that is created. I think I prefer the Service Loader idea -- to load, once, the global tag initializer -- since that would ensure that invariant global settings are only set once.

[donbourne]

Also, if we really do want to insist that the global tags are immutable, then perhaps this is all moot -- MetricId constructor could just be changed to cache the result from the call to MP Config (if MP Config is present), and the performance problem would go away.

[aseovic]

@donbourne I think the spec should define that the global tags are immutable, but if you look at previous messages, you'll see that one of the issues we ran into is that the config is the only way to specify global arguments at the moment, which is insufficient -- we need to be able to define some global tags for all the metrics published within the process, and I'm sure there will be other framework authors who need the same thing.

The reason for an additional constructor is driven by the first goal @jmartisk mentioned in this issue, which is to allow MP implementations to control, well, the implementation of the spec, instead of forcing them to rely on the default implementation. There are many places where global tags could be initialized and cached, forcing everyone to use the implementation within the MetricID constructor just doesn't feel right. We should keep the behavior for backwards compatibility, but open the door for MP implementations to override it if (and when) they want to.

I think what I'm proposing would address all three things:

1. Performance concerns with the current implementation
2. The ability for any component in the ecosystem to define additional global tags
3. Preservation of backwards compatibility

However, I'm not sure if what I'm proposing is clear, so I'll submit a PR and we can discuss when we have the code in front of us.

[donbourne]

Thanks @aseovic , I've added a comment to your PR. code always helps clarify :-)

[ebullient]

Is there more to do with this one?

[jmartisk]

I'd say we can consider this fixed for 3.0; should there be any other problematic cases, let's open a new issue.