Proposal for global tags refactoring

[aseovic]

Signed-off-by: Aleksandar Seovic aleks@seovic.com

Partial fix for #506

[eclipse-microprofile-bot]

Can one of the admins verify this patch?

[jmartisk]

ok to test

[jmartisk]

Also @jmesnil should probably have a look from the WildFly side, we need to make sure it will be acceptable for multi-app servers

[jbee]

I had a look at this PR and have let it sink in a bit. To me it feels like global tags get even more complicated and hard to see though and implement correctly.

If the only way to get a non default global tags provider when constructing a MetricID is to pass it to the constructor why not just expect the caller to simply pass the global tags along other tags as Tags. The hard part of consistently adding global tags to every single place where a MetricID is created is the same. The concept of a provider seems unnecessary. I also feel there is a high risk of painting oneself into a corner where you have to create a MetricID at a place where you have no access to the provider or where it is undecidable. In effect this e.g. means the API cannot contain a single new Metric invocation as those would become invalid in case a custom global tags provider is required for correctness of the resulting ID (multi app container). This is getting messy fast.

My best idea to address global tags is this: As with any situation that feels hard to decide change the system so you don't have to. I think MetricID should stay out of global tags business. As far as the class is concerned there are only tags. Global tags are a small feature on top of ID creation that should be handled outside (on top). My idea would be to have the registry be responsible. That means it does add global tags when registering metrics and "assumes" the same set of tags when doing a lookup. This can be solved differently implementation-wise as long as the registry callers see global tags present in the map keys and the metadata returned by the registry. This does mean that different MetricID keys do lead to the same metric as one could do lookup with or without global tags but this is highly unlikely as all the global tags handling is "virtual" and only relevant within the registry implementation. Rest of the code pretends there is no such concept. If you pull metrics out of the registry you do observe the global tags and that is all I can see global tags are supposed to do. As this would be per registry this works smoothly for single and multi application.

Moving the global tags to the registry would also unify the exiting discrepancy between using registries methods accepting String and Tag[] with those accepting MetricID. Both would not expect the caller to provide global tags but both would assume their presence. Meaning the registry matches as if global tags would have been provided in case they are missing. I think currently this is under-specified with regard to global tags. It is just very likely that the implementation does use MetricID internally which then adds global tags.

[ebullient]

Ok.. I would like to ask one quesiton: what do you expect to be in a global tag?

I would only expect global tags to express things that apply to the process as a whole. In the case where you expect multiple apps in a process (which is not typical for microservices anymore), then app-specific tags, by definition, would not be global tags, and wouldn't apply here, right?

Also, I would expect global tags to be applied to ALL metrics, because they are descriptive of the process creating the metrics, not for any individual metric. The set of tags you'd define as global tags is fairly small, isn't it?

[jmartisk]

I think @jbee's suggestion of moving the responsibility from MetricID to the registry kinda makes sense, but it poses a few challenges that need thinking.

Suppose you register a metric (with no tags specified), the registry automatically appends the global/app tags, so it basically changes the MetricID (the MetricID class itself is immutable, so it would need to create a copy including that tag). Then you try to look up that metric using registry.counter(MetricID). How will that work? The registry will have to again "sanitize" the passed MetricID that you pass to it? But then you get back a metric which actually has a different MetricID than what you passed to the lookup method, which might be unexpected. The question is if this is acceptable, or just plain wrong. But the good thing about this approach is that it's all left to the implementation and we wouldn't need any SPI.

Another option would be to move the responsibility to exporters instead and not care about global tags at all in the registry or MetricID. Exporters would be just supposed to append the global tags when exporting. The downside is that the program code won't see the tags. Again, it's hard to tell whether that's acceptable. The _app tag would need a different handling because it's probably hard for an exporter to determine which application the metric belongs to (if there is a shared registry for all applications, as is the case in WildFly for example), but this could be solved by adding the _app tag by the registry during registration.

[jmartisk]

I would only expect global tags to express things that apply to the process as a whole. In the case where you expect multiple apps in a process (which is not typical for microservices anymore), then app-specific tags, by definition, would not be global tags, and wouldn't apply here, right?

AFAIU global tags are generally meant in cloud environments to be able to distinguish different instances of a service.

App-specific tag _app is just for application servers to be able to distinguish different applications. It doesn't necessarily connect to global tags, it's a different mechanism, and in a true microservice environment it doesn't serve any purpose.

[jbee]

you get back a metric which actually has a different MetricID

As a Metric instance does not have access to its ID this is a non issue as far as single metric lookup is concerned. Maps or sets of IDs will always contain global tags. So the most "odd" case I can think of is when you use MetricFilters. It has to be defined if the MetricID passed to them is with or without global tags present. If left without you cannot filter on global tags, if these have global tags (which feels more "correct") using equals might not do what you want. A way out could be to add a method to the registry:

MetricID withGlobalTags(MetricID metricID);

The registry will have to again "sanitize" the passed MetricID that you pass to it?

There are certainly ways to minimize the unification of with/without global tags but conceptually yes, I'd assume that in general any MetricID passed to the registry can skip global tags and the registry acts as if those are present. Same goes for methods accepting String and Tag[].

Another way out: Add another field to Metadata that specifically holds the global tags. Any MetricID is always without global tags. This fits nicely to global tags having a role in exports and such but are of no relevance for using metrics. Again the registry is responsible for adding the global tags to the Metadata as stored in the registry.

[jbee]

It dawned on me that if MetricID should hold global tags at all the cleanest thing is to differentiate these from other tags by keeping them in a separate field. This will help reduce the confusion between usual tags and global tags, reduce work of adding global tags and allow to even strip global tags.

For MetricID I suggest to add a globalTags field and:

public MetricID withGlobalTags(Tag... globalTags) {
  return new MetricID(name, tags, globalTags);
}

which can be called with no arguments to strip the tags or with arguments to "set" them. As MetricID is immutable this naturally does not change the instance invoked upon but returns a new instance with globalTags field changed.

When it comes to comparing MetricIDs I suggest to not consider global tags part of the ID - they are more like annotations. So equals and hashCode would ignore them. I know such a suggestion will trigger some people. So in case global tags are considered for equals and hashCode I suggest to to add equalsIgnoreGlobalTags which is same as this.withGlobalTags().equals(other.withGlobalTags()).

This improvement can be made independent of the solution chosen to provide the global tags. It would work with provider or in case the registry is responsible or if global tags are stored as part of the Metadata (in which case only reason allowing to add the global tags to an ID would be to present full IDs to some export consumer and alike just because this might be the easier way to write the code).

[ebullient]

I really don't like global tags being part of the metrics ID at all. They can be added at the last possible minute by the exporter (they are common to all metrics). Requiring this to be set or read when constructing MetricID has made it really difficult for me to adapt the MP Metrics API on a different system.

I otherwise agree with @jbee .. that would work for me.

[jbee]

They can be added at the last possible minute by the exporter

If global tags are only relevant for export/API it would be a good way around the issue

Requiring this to be set or read when constructing MetricID has made it really difficult for me to adapt the MP Metrics API on a different system.

Mixing data and behaviour has this tendency. I am glad you can confirm this from an actual case so the discussion does not have to be about theoretical concerns any more.

[jmartisk]

I really don't like global tags being part of the metrics ID at all. They can be added at the last possible minute by the exporter

AFAIK this solution was considered when designing the whole MetricID concept. The perceived problem was that then from the program side, you wouldn't see the full list of tags. Whether that's really a problem, I don't know. Perhaps it's not, application's code probably shouldn't depend on it.

Maybe there were some more problems with it, I don't clearly remember now. But I tend to agree with your view now. It would probably be the least painful way to go. Anyone has any objections to doing this?

[ebullient]

We can have a completely different discussion about whether or not MetricID should ever have been exposed. It’s an internal detail of the MetricRegistry, IMO. I am not sure what the rationale was for introducing the MetricID — there are other ways to pre-filter or reduce what is returned that would be as cheap as what was done without using the MetricID concept.

As a reference, look at what Micrometer did with search: Search.in(registry).name(“exactName”).tags(...).counter() and variants. All of those do filtering, without exposing the registry’s internal notion of metric IDs.

[jbee]

We can have a completely different discussion about whether or not MetricID should ever have been exposed. It’s an internal detail of the MetricRegistry, IMO.

If it would not be for global tags the class would have been just a record grouping metric name and tags. There really is nothing internal about that. The registry might use that internally or not. Without the behaviour part of global tags its just a form of how API communicates with the outside world and having it or not would neither create a problem nor solve any. Surely one can design the API to avoid records in favour of e.g. a fluent builder API but that would not lift or put a burden on the user. Having a class helps to reduce argument count and avoid duplication of same groups of arguments and to communicate a concept of identify to the user.

[ebullient]

Y.. but that class imposes implementation choices that may not be ideal, and the global tag behavior is suspect. :-P

[donbourne]

I think arbitrary global tags could be moved out of Metric ID, but we'd still have the app tag from mp.metrics.appName to deal with. That app tag isn't just decorative as servers that have multiple applications use the app tag to differentiate metrics with otherwise identical MetricIDs. If the app tag has to stay here then there's not much gain out of moving the arbitrary global tags to another class.

[jbee]

servers that have multiple applications use the app tag to differentiate metrics with otherwise identical MetricIDs

They exist in different registries though - so there might be no need for the tag after all, except for export where the context of a registry or object context in general are no longer available.

[jmartisk]

They exist in different registries though - so there might be no need for the tag after all, except for export where the context of a registry or object context in general are no longer available.

You can't assume that, it's an implementation detail - for example WildFly uses just one registry for all applications

[ebullient]

You can't assume that, it's an implementation detail - for example WildFly uses just one registry for all applications

Exactly. It is an implementation detail. So why is it in MetricID again? Why can't the registries that rely on this add the tag at ID creation time?

Also:

"It is allowed for application servers to choose to not add the _app tag at all, but in that case, metrics from two applications on one server can clash as no differentiator (by application) is given."

How is an app server going to opt out of creating the _app tag if it is baked into MetricID?

[kenfinnigan]

I've tried to read through the previous history on this, as I wanted to comment after stumbling across https://github.com/eclipse/microprofile-metrics/blob/master/api/src/main/java/org/eclipse/microprofile/metrics/MetricID.java#L127 recently.

+1000 to global tags being applied at the export time only. There's absolutely no need for applications to know, or care, what global tags may or may not be present on every metric.

As for application name, why can't it just be something that an implementation does if required? Currently, the API has encoded the config call which means single deployment runtimes are making an unnecessary config call with no way to avoid it.

There are many cases I'm seeing, not just in Metrics, where what's included in the spec is encoded into the API classes. We need to revert that thinking and instead define behaviors in the spec, but only enforce them in a TCK and not the API.

[jmartisk]

I've submitted a PR https://github.com/eclipse/microprofile-metrics/pull/595 that removed global/app tags handling out of MetricID and moves the responsibility to implementations (most likely exporters). Feedback is welcome.

[donbourne]

I've commented in the PR -- the addition of the _app tag has to happen at or before metric registration time as it differentiates the same metric used in two different apps. I think we need to investigate if it would be possible to add the _app tag at registration time by each impl.

Note that currently all of the "global" tags can actually be app-specific, since MP Config allows for config settings to be specified in a file in the app.

So I think perhaps both "global" and _app tags may need to happen at or before metric registration time.

[aseovic]

I think arbitrary global tags could be moved out of Metric ID, but we'd still have the app tag from mp.metrics.appName to deal with. That app tag isn't just decorative as servers that have multiple applications use the app tag to differentiate metrics with otherwise identical MetricIDs. If the app tag has to stay here then there's not much gain out of moving the arbitrary global tags to another class.

I'm not sure -- presumably, different apps will have different metrics registries, and the server could easily register different global _app tag with each registry. After all, the knowledge/awareness of multiple applications belongs to the server, not to the application.

[aseovic]

Sorry, I've been quite busy over the last few months and this fell off the radar, but glad to see that the discussion continued and the progress has been made.

Just to confirm, I agree with the consensus you've reached -- it absolutely makes sense to remove global tags from the MetricID completely and put onus on either registry or the "exporter" to add them. In vast majority of the cases the application is unaware of them, and can't use them to query metrics anyway.

I will close this PR and we can continue discussion in #595.