Move global/app tag handling out of MetricID

[jmartisk]

possible solution for https://github.com/eclipse/microprofile-metrics/issues/506

[donbourne]

There's a problem here. Moving the time at which "global" tags are added to the exporters is only a small behavior change, but moving the time at which "_app" tag is added to the exporters means that the "_app" tag can no longer be used to differentiate the same metric being used by 2 different apps in the MetricRegistry. That means that apps would have to share metric instances, which kind of defeats the whole purpose of the _app tag.

[donbourne]

Perhaps the implementations could add the "_app" tag at metric registration time? @Channyboy , would that be possible?

[Channyboy]

There's a bit of problem for multi-app MP Runtimes.

With a runtime that can support multiple apps it would be necessary to associate Metric/MetricID with the MP Config values that can be defined uniquely for each application through the microprofile-config.properties. The MP Config specified tags (i.e mp.metrics.tags and mp.metrics.appName) can be resolved before retrieval and registration. The resulting registered metric's MetricID will contain the global and app tags.

This, however, introduces a problem when using MetricFilters in a specific way.

MetricID metricID= new MetricID("name", new Tag("key","value");

Counter c = metricRegistry.counter(metricID);

metricRegistry.getMetrics( (metricID, metric ) -> {myMID.equals(metricID) ;})

Trying to match exactly the MetricID used will fail due to the registered MetricID containing additional tags defined through MP Config.

[jmartisk]

@Channyboy, but this PR says that generally implementations won't append tags to metrics automatically, so comparing metricIDs directly shouldn't be a problem, no? The _app tag will be added only during export. The implementation will need to keep track of the origination application when registering a metric, but that information will need to be somewhere outside the MetricID itself.

[kenfinnigan]

I'd certainly prefer us to keep the spec neutral to multiple app runtimes.

Is it possible for implementations to create a separate registry for each application "under the hood"?

[jbee]

Is it possible for implementations to create a separate registry for each application "under the hood"?

Yes, we do that.

[kenfinnigan]

Doesn't that make the concerns around app tag and metric ids moot if application servers can separate each app's metrics into a separate registry within the implementation?

[jbee]

Doesn't that make the concerns around app tag and metric ids moot if application servers can separate each app's metrics into a separate registry within the implementation?

If they have chosen this path, yes. But others have chosen single registry as solution. Can't say if this is just a choice for them or of some limitation makes this the only feasible solution for them.

[donbourne]

I get the problem @Channyboy is describing for MetricFilters...

Currently the _app tag and global tags (collectively I'll call them the "config-tags") are present in the MetricID alongside the programmatically supplied tags. When I create a MetricFilter and use it in one of the MetricRegistry's methods to retrieve Metrics I expect my match method to be called for every Metric in the server. That's pretty straightforward.

This PR proposes that config tags are not part of the MetricID. I think that means that, for MetricFilters, the server would need to avoid calling MetricFilter.match method with any Metric that belongs to an app other than the "current" app (where app detection is basically derived from the Thread Context Class Loader). That's a bit more complicated, but could work.

If a MetricFilter is used with MetricRegistry methods outside of the scope of an app thread then multiple metrics with identical MetricIDs are possible, with no way for a MetricFilter to differentiate those metrics. That seems broken, and I'm not sure how to resolve that.

[jmartisk]

I think that if an application server wants to allow clashes between metricIDs of metrics from different applications, then it needs to choose the "multiple registries" (one per app) approach to avoid problems.

But I think this is very much an edge case, because while it is common to horizontally scale the same application to multiple AS instances, I don't see why one would deploy the same application archive multiple times to one AS instance. And if it's not the same application archive, then it should not contain application metrics with the same ID.

[ebullient]

This PR proposes that config tags are not part of the MetricID. I think that means that, for MetricFilters, the server would need to avoid calling MetricFilter.match method with any Metric that belongs to an app other than the "current" app (where app detection is basically derived from the Thread Context Class Loader). That's a bit more complicated, but could work.

I'm not sure that is what we are saying. What we are saying is that looking up the app id to set that tag is not something that the MetricID class does. There isn't anything preventing a runtime that supports multiple apps in the same registry from adding or including that tag in other ways.

[donbourne]

@jmartisk , I'm not keen to move to a separate registry per app -- that adds new complexity for our impl that I'm hoping to avoid.

@ebullient , I get your point -- metric IDs could still be stored in the MetricID even if they are looked up elsewhere.

I had a discussion with @Channyboy , and we have a suggestion that allows for differentiation of metrics by _app tag while not requiring the lookup of the config-sourced tags to happen during MetricID construction:

• add all config-sourced tags at Metric registration time
• impl is free to cache the config-sourced tags at time of first lookup to improve performance. App tags obviously would have to be cached per-app.
• add methods to MetricRegistry so that it's possible for someone wanting to match to name and ALL tags of metrics in MetricFilter.matches(...) calls to get the config-sourced tag values
  • MetricRegistry.getGlobalTags
  • MetricRegistry.getAppTag

Would that work?

[kenfinnigan]

I might be missing something, but aren't we trying to remove global tags handling from the API of the spec?

[jmartisk]

I must say I don't like the idea of adding MetricRegistry.getAppTag into the API that would only make sense in application servers, and only those who decide to implement this stuff in a particular way (because the spec shouldn't quite mandate any specific behavior in this regard). I think we should be aiming to remove all global/app tag handling from the API completely. That would also allow us to drop the MP Config dependency from the API, which would be a nice bonus for simplifying things.

@donbourne, do you really need to be able to handle metric ID clashes between applications in one shared registry? I mean, that sounds like complicating things just to be able to support using an anti-pattern.

[jbee]

I think we should be aiming to remove all global/app tag handling from the API completely. That would also allow us to drop the MP Config dependency from the API, which would be a nice bonus for simplifying things.

I was about to suggest something like this. It feels like using global tags were abusing the concept of tags. Tags that are present on every metric aren't semantically tag but something else. Similarly the app tag feels like an abuse of tags to solve another issue within application servers supporting multiple applications. I also feel the cleanest is to get rid of global tags, maybe add something specifically to the export and have servers solve multi-application clashes by other means.

[donbourne]

@kenfinnigan ,

I might be missing something, but aren't we trying to remove global tags handling from the API of the spec?

Yes. I'm just contemplating where those tags should get introduced in the vendor impls. The 3 choices are 1) registration time 2) export time, 3) don't care.

The reason I was leaning toward "registration time" was because our current impl feeds EVERY metric to calls that take a MetricFilter (which necessitates having the _app tag in the MetricIDs passed to the MetricFilter).

Let me try another proposal that keeps things easy for single-app vendor impls without significantly changing multi-app vendor impls, and which would work regardless of how many app metric registries the impl has:

• add global metrics to the output (not to the MetricID) at export time
  • these are just "decoration" anyway
• add _app metric, if configured, to the MetricID at registration time.
  • helps differentiate metrics with the same name/tags in the output
  • helps in impls which feed EVERY metric to MetricFilters.

@jbee , if you have multiple apps I think you can't do away with app tags. If I have a metric "A" with value 5 in one app, and metric "A" with value 7 in another app, I need the app tag to differentiate...

A{_app=X} 5
A{_app=Y} 7

[jbee]

As long as each app can only see its own metrics they do not clash given you use one registry per app. Obviously outside of such scope a tag is needed. As far as I am aware the only real point where metrics of all apps can be observed would be the export where the tags can be added at export time. This obviously falls flat as soon as you try single registry solution. I just wanted to say that it can be solved for application server without the app tag at registration time.

[kenfinnigan]

@donbourne I think I'd be ok with _app added at registration time if it's "under the covers" and something only multi app runtimes need to worry about.

i.e. Spec doesn't say or verify anything about that behavior.

[donbourne]

@Channyboy , can you please give this a quick try on Open Liberty (adding app tag to MetricID at registration time)? The point you mentioned about MetricFilter not matching with MetricID.equals(...) we can likely just document in the usage guides.

[aseovic]

Just trying to catch up with this discussion, and it seems like the only major point of contention is _app tag handling.

I tend to agree with @kenfinnigan, @jbee, @jmartisk and @ebullient -- _app tag feels like an attempt to handle a corner case that really doesn't belong in the spec at all. It should be up to the servers that support multiple applications to add whichever tag they want to add to disambiguate the metrics across applications.

We (Coherence) add a bunch of global tags to all metrics in order to disambiguate them across clusters, members, machines, racks and sites, as well as services/apps/roles, and we don't need separate, well-known tags in the spec to support that. More importantly, our metrics are effectively vendor metrics, and many will have the same name across multiple clusters (coherence_cluster_size, for example). But, we are aware of the context we are exporting them from, and can easily add cluster_name tag to disambiguate them.

There is absolutely no reason any app server can't do the same, one way or another, so defining a special _app tag in the spec at all seems very wrong.

[aseovic]

Just reviewed the proposal and have a few comments.

Removing global tags handling from the MetricID is a good first step, but I don't think it is sufficient. The spec should also define how the global tags can be added by any application component: MP server implementation, other libraries/products within the app (which would cover my use case), and even the application itself.

Unless those semantics are defined in the spec, I will have to:

a) rely on the MP Config vendor to even give me the extension mechanism I can use to add my own global tags, and b) implement vendor-specific integration for each MP implementation we want to support

I would much rather have an extension mechanism defined in the spec that allows me to define global tags in a way that will be vendor-neutral, so I can write a single MP Config extension that is guaranteed to work with any vendor's implementation. After all, isn't that the point of having a standard spec to begin with?

Whether that means adding global tags to the registry directly (preferred) or something else (ServiceLoader or CDI discovery, as discussed earlier) is less important -- as long as there is a well-defined mechanism that allows me to do what I need to be able to do.

[ebullient]

Whether that means adding global tags to the registry directly (preferred)

+1

Comparison in micrometer: registry.config().commonTags()...

[kenfinnigan]

+1, global tags should either come from a defined config key, or be set directly on the registry

[jmartisk]

Rebased. It looks like this is ready to merge, if there are more related concerns, let's handle them separately

[aseovic]

I've opened #600 to track the need mentioned above which hasn't been addressed at all by this PR.