Move global/app tag handling out of MetricID

[jmartisk]

possible solution for #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

[donbourne]

In trying to implement this on our multi-app Open Liberty server, we're noticing

• we intend to add the _app tag at registration time
• MP Config currently allows config values to be application specific, which means the "global tags" are potentially app-specific tags.
• if we are going to add the app tag to the MetricID at the time of registration and want to continue to support app-specific global tags (because there's no real reason not to), it would make sense to put the "global" tag values into the MetricID as well.

So question is -- is there any reason not to allow all of the config-sourced tags (global and _app) to be put into the MetricID at registration time? If not, we should remove the sentence above that says global tags must not be added to the MetricID objects.

[ebullient]

I think you just stop reading global tags within an app context.. ignore them. Global tags are global tags. If you want some other kind of app-common tags, then do that separately.

[jmartisk]

Yeah, reading global tags in the context of an application and thus potentially allowing different apps to have different global tags, that sounds wrong.

[donbourne]

Fair enough... @Channyboy was looking last night to see if we could call MP Config to get the global tags purposely without any application context. At least at first glance it appears that we can do that as long as we choose the right classloader (ie. not the TCCL) to pass to MP Config to do the lookup. We may want to clarify in the spec that we explicitly don't want application context config for the global tags.

[jmartisk]

@donbourne applied that via 46f7119, looks good?

[aseovic]

Hmm... I think we may need (conceptual) separation between global tags (apply to all metrics across all registries within the process), and registry-level tags (apply to all metrics within a single registry).

The former can be easily mapped to latter (and in most applications they will likely be one and the same, if we switch to a single registry per #384 discussion), but there may be situations when global tags may map to multiple registries (such as multi-app servers @donbourne described), and there may be situations where you want per registry tags that are not truly global (such as app name discussed earlier).

I don't think this impacts this PR per se, but it is something that should be defined by the spec.

I do think this PR should add support for registry-level tag management in order to be complete, though.

[ebullient]

micrometer does this because of it's composed registry structure. Same mechanism is used (registry.config().commonTags() is one way, there are others), just depends on which registry you apply it to (global/top-level composite, or an individual registry). Significant difference is that a registry in micrometer is more closely aligned to an exporter in MP Metrics instead of being associated with a scope. I think someone needs to explain to me the value of scopes.. because outside of strictly hierarchical names, I don't get it (not a conversation for this item, however. ;) )

[jmartisk]

I'd say that if an implementation needs registry-scoped tags, it's not a problem to add that as a vendor-specific thing, but I wouldn't bake it into the spec, especially when we're considering switching to a single registry

[aseovic]

I'd say that if an implementation needs registry-scoped tags, it's not a problem to add that as a vendor-specific thing, but I wouldn't bake it into the spec, especially when we're considering switching to a single registry

I disagree -- it's not about what any MP implementation needs, but what the MP users need. What you are suggesting (an optional, "vendor-specific" way of defining tags at the registry level) is exactly what I want to avoid.

If that happens, the best case scenario is that I have to implement different integration for each MP implementation, which is both cumbersome and pointless. Worst case scenario is that I can't add the tags I need to add because the vendor didn't provide a way for me to do it to begin with.

Again, I am not an MP vendor, and while I may be able to influence one MP vendor (Helidon), I can't really influence what anyone else does. This feature has to be defined at the spec level so I, and any other spec user who may need it, can rely on it to be able to add our "global" tags programmatically to one or more registries.

Switching from multiple to a single registry doesn't really change that need, btw. It just makes it easier for me to implement if there is a single registry to deal with.

[jmartisk]

Could you send a separate PR for that if you think it's needed? I don't have a clear idea about how exactly you want to do this. I'd say let's get this one merged and not over-complicate it.

[Channyboy]

Global tags are Global. And it can be up to vendor to append mp.metrics.appName tags to the MetricID at registration. I'm good with that 👍

[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.