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

[jmartisk]

I think I see a problem for application servers with multiple applications here. If the DefaultGlobalTagsProvider provider is a singleton and it caches the global tags once, there is no way we can get different tags for each application in case that all applications share the same singleton instance, which they will if they use a shared class loader to load MP Metrics classes.

So how could we differentiate between apps? Perhaps we could solve this by not caching the returned tags from providers found by ServiceLoader, and instead letting the custom providers do the caching themselves? Each time getGlobalTags is invoked, the custom providers would need to be called again. And the custom provider would know what application it is serving so it could return the right _app tag. We'd also need to make sure that _app tags from the custom provider aren't overwritten by the DefaultGlobalTagsProvider.

[donbourne]

Perhaps we could solve this by not caching the returned tags from providers found by ServiceLoader, and instead letting the custom providers do the caching themselves?

That sounds like a good approach. Would be good to see different servers try this to see if it works well for single and multi app servers.

[aseovic]

@jmartisk The default implementation is meant to be just that -- the default.

I believe it is reasonable to expect app servers to provide their own implementation, which takes all these things you mentioned (multiple apps, using multiple class loaders) into account and pass it to the new 3-arg constructor when creating MetricIDs.

As a matter of fact, I think everyone should do that, including frameworks that are not app servers and don't support multiple apps in the same process. The default implementation is there purely for compatibility reasons, to avoid breaking anyone right away. Maybe we should even deprecate the 2-arg constructor, so everyone knows they should not use it long term.

[aseovic]

@jmartisk I moved the singleton instance that caches global tags lookup from DefaultGlobalTagsProvider to MetricID.

This allows DefaulGlobalTagsProvider to be reused (if we want to allow that) by the implementations, although we'd have to make it public to support that use case. App servers would be able to create an instance per application, and pass it to the new MetricID constructor.

I've also marked old constructors as @Deprecated.

[jmartisk]

I strongly disagree with deprecating the original two constructors. These are being used by application developers.
Why should we force application developers to use a constructor that takes a GlobalTagsProvider? That thing should only be concern of the implementation, not app developers.

I think a suitable solution would be that the MetricID constructors use the ServiceLoader to see if there are any custom GlobalTagsProviders, and if yes, use them; if not, then use the default one.

With the currently proposed code, if you use the DefaultGlobalTagsProvider, it will also locate and call the custom ones, which is not what one would expect to happen if using the default. Even worse, if your custom ones return different tags over time, the default provider will cache the result after first calling the custom ones, which is unexpected.
Either only the default should be used, or only the custom ones.

[donbourne]

Why should we force application developers to use a constructor that takes a GlobalTagsProvider? That thing should only be concern of the implementation, not app developers

👍

[aseovic]

No strong feelings on deprecation, either works for me. I'll back out the change.

The rest of it is not so straight forward, though.

I don't think passing ServiceLoader directly is the right thing to do. It really shouldn't be either/or (default or custom), because I'm not sure how the spec would even define that (each implementation would be free to implement behavior any way they want, which would hurt portability and defeat the purpose of having a spec in a first place). It is also inefficient, as it would suffer from the same issue current constructor has, which is lookup and calculation of all global tags every time new MetricID is constructed.

I think we need to refocus on defining the spec first, and then we can implement it accordingly.

1. Do we, or do we not want to initialize global tags only once? We seem to be going back and forth on this, based on discussion above? Yes, I understand that the current spec does not define that behavior, which caused the original complaint about current implementation's inefficiency, but I was under the assumptions that we all agreed that calculating a set of global tags once and caching them makes sense, for a variety of reasons. Let me know if that's not the case.

2. Do we want to provide a mechanism other than the config for spec users to provide global tags? This is where the notion of having ServiceLoader-based mechanism came from, and the reason I started looking into this in a first place, as we have a real need for that feature. If we want it, that behavior/feature needs to be defined by the spec.

3. If we have both ServiceLoader and config-based mechanism to provide global tags, what should the order/priority be? That's the reason I chose not to use ServiceLoader-based implementation for the config-based tags, as it allows us to control the ordering, but again, this needs to be defined by the spec.

So let's answer the questions above first, and once we do I'm happy to change the implementation accordingly.

[jmartisk]

Do we, or do we not want to initialize global tags only once?

We do want to cache global tags - or at least allow the implementation to do that if they choose to - it shouldn't be mandatory.

Do we want to provide a mechanism other than the config for spec users to provide global tags?

I assume that by "spec users" you mean the MP Metrics implementation (not application developers), right? Of course we want to open ways for the implementation to do things as they want, in addition to the default behavior defined by the spec. That's why I think the ServiceLoader approach is a way to go.

On the other hand, we probably don't want application developers to have to be concerned about global tag providers. That's why I think we don't need a new MetricID constructor or deprecate the original ones. From the app developer point of view, defining the tags should be a matter of just configuring the server.

If we have both ServiceLoader and config-based mechanism to provide global tags, what should the order/priority be?

I think one problem here is that these two mechanisms you mention are not necessarily two completely separate things - a ServiceLoader approach may be needed to implement a customized config-based mechanism.

If an implementation wants to do the MP Config-based mechanism (which they have to to pass the TCK), they might need to use a different piece of code for that than the code which we supply as default. Examples when this will be needed:

• an application server where the returned global tags depend on the application which calls the tag provider.
• or if it depends on some vendor-specific configuration mechanism other than reading the mp.metrics.tags property
  So in this case they would need the ServiceLoader approach to plug in their own implementation of a config-based mechanism.

So what I think would be a solution that fits (I hope) all use cases:

• Provide a default GlobalTagsProvider that looks into MP Config only once to get the mp.metrics.tags property, generates tags based on that, caches them, and each call to it will return these cached tags. This should work for most single-app microservice runtimes, at least those that just stick to the spec only and don't provide any custom mechanisms on their own.

• Open a way to provide a custom GlobalTagsProvider through ServiceLoader, and if any such provider is present, this provider (providers) will be used instead of the default one. This will be for the implementations that need to do something specific, including considering multiple applications (looking at the mp.metrics.appName property, checking the TCCL or some thread-locals, or whatever else to determine the current application) to generate tags. Such custom provider would, of course, also need to take into account the regular mp.metrics.tags property to stay compliant.

• The MetricID constructor will always either call the default provider, or the ones from ServiceLoader. It won't cache the obtained results, potential caching will be responsibility of the providers (the default one will do it always, the custom ones can choose to do it).

• With this approach we would remain backward-compatible for most implementations, because we would be providing the default provider which reads the mp.metrics.tags property

• There remains a question whether the implementation (not application directly) might want to use a different GlobalTagProvider each time it is registering a metric. Because that would be a reason to allow passing it to the MetricID constructors. But I don't currently see any reasonable use case why an implementation would want to do this - if they want tags to be somehow "dynamic" (whatever that means exactly), these dynamicity can be implemented within the providers themselves rather than by switching between different providers.

I hope that clears up my views on this :)

[donbourne]

I agree with almost everything you said @jmartisk . One thing I want to poke at is whether we truly need GlobalTagProvider at all. Your two stated use cases are:

an application server where the returned global tags depend on the application which calls the tag provider.

Here, Liberty has been able to just use MP Config to read application-specific versions of each tag.

or if it depends on some vendor-specific configuration mechanism other than reading the mp.metrics.tags property

do we have a true request for allowing a config mechanism other than reading from MP Config? I think we need clarity on why a component wouldn't just add its component-wide tags to the array of tags when creating a new metric. If that truly is a need (component-wide tags) then I agree with need for GlobalTagProvider.

[jmartisk]

do we have a true request for allowing a config mechanism other than reading from MP Config?

We don't have a request here, but some implementations may choose to do introduce their own mechanism on top of the spec, for example WildFly has got its very specific configuration model (which is based neither on MP Config nor system properties), and they may choose to read global tags configured within that model as well

And determining the application name might also need some vendor-specific mechanism, it might not be just about reading mp.metrics.appName, especially if a multi-app server does not support separate configuration spaces per each application (or chooses to not use mp.metrics.appName at all, that's just a recommendation, not requirement). In such cases, the implementation may, for example, need to check the TCCL or some thread-local variables to be able to determine which application is the active one.

[donbourne]

This method is essentially saying you can ignore the globally configured tags and instead use what the caller says are the global tags for this metric. But then for all metrics that don't come from this caller you'll have a different set of global tags. I'm not sure I understand the use case for this.

[aseovic]

It simply guards against the case when there may not be a global tags provider, but if you think we should require one, I'm ok with that too.

I guess that probably makes sense, considering that any spec-compliant implementation will have to support at least config-defined global tags. I'll make the change and document that GlobalTagsProvider cannot be null.

[aseovic]

Done.

[donbourne]

Given the global tags are meant for the implementation to provide, why don't we make the DefaultGlobalTagsProvider look up the impl of GlobalTagsProvider using service loader and use it to fill in the metrics. That would avoid us having to expose the GlobalTagsProvider in the MetricsId API, which I think is better as app developers shouldn't be concerned with that concept.

[aseovic]

App developers may not be concerned, but MP Metrics implementors certainly should be, as it allows them to optimize things.

Otherwise, everyone has to rely on the default implementation, which is no better than the current solution. The original stated goal of #506 was to allow implementations to have better control over the implementation of the spec, but somehow the discussion has devolved from that into which constructors we need and should have on MetricID :-(

In any case, if you feel strongly about not exposing new constructor to everyone, we can make it protected to allow implementation to use it, but that would force each implementation to extend MetricId for no other reason, which I believe is not really necessary.

[jmartisk]

The implementation will provide the GlobalTagProviders available through ServiceLoader, so that's how they will be picked up. I don't think we need a special constructor that takes a GlobalTagProvider argument. Certainly applications don't need it (I hope we all agree on that), but I don't think the implementation needs that either.

[donbourne]

@aseovic, I agree with your point that the goal is to give more flexibility to implementations to implement the spec their way. My concern is just that we're exposing things that only an implementor should be concerned about in the API that app developers will use. That worries me because it would confuse app developers that don't know what to do with this parameter. I think @jmartisk and I are on a similar point -- that GlobalTagProvider is something only the impl should ever see, which is why I like the idea of using Service Loader to load GlobalTagProvider within the DefaultGlobalTagsProvider. That would avoid GlobalTagProvider being something on the classes that app writers interact with.

[jbee]

As far as I understand the main pain point of current global tags implementation are the bad performance implications and the lack of control for implementers to solve this more elegantly. In this context I fully agree that such logic never should have been in the API. Adding the service loader indirection does allow more control but also opens a new problem vector. If service locator is used implementers have to assume it is used by users which will only give them the option to hope that the user does implement the caching of reading the config which is almost unlikely. It also opens up possibility for users to make the implementation non conforming. Giving the user both a configuration key and an abstraction feels unnecessary duplication and burden both for users to understand and for implementers to handle the complexity this introduces. I made a suggestion #560 (comment) to entirely move global tags out of MetricID class which I feel removes all the pain points and gives all control back to implementers while users are not bothered with the detail of global tags except that they can use the configuration to set them.

[jmartisk]

If service locator is used implementers have to assume it is used by users

I don't think we need to be concerned with that. We can just put in the spec that this is meant for implementations, and bundling a provider directly in your application results in undefined, non portable behavior

[jbee]

We can just put in the spec that this is meant for implementations

If this solution goes forward I'd consider this a very important point to include.

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