Don't deprecate getMetadata() and getMetrics() in registry

[rmannibucau]

These methods are still the preferred way to access all set in bulk so no real reason it is deprecated to use getNames then iterate to do N getMetada(n)/getMetrics(n)

[jmartisk]

This was originally suggested by @jbee and I think the main reason was that the maps can change over time and the issues arising from that.

Speaking of which, I'm not sure if we specify whether the returned maps from getMetadata and getMetrics should be

• a copy of the state at the time of calling, so if the map changes later, the returned copy is unaffected
• a unmodifiable view of the state, that means when the map changes, the view will reflect these changes

So I think, perhaps we should decide for one of these options, and if this is then specified clearly, then there would be no reason to deprecate these methods?

[rmannibucau]

Hi @jmartisk , all impl assumed it was a copy so guess it is the way to go. Personally I understood it as the snapshot representation in some other Metric instances. Maybe just a javadoc thing but its usage is quite key and all impl use that in exporters (there is no other valid way to impl it) so if an user want a custom exporter he must be able to access that in a "stable" way IMHO.

[jmartisk]

This is a very valid point that we seem to have missed. Implementations are using it to be able to perform the export, they need a snapshot (immutable) of the metrics and metadata to be able to perform the export correctly. Care to submit a PR to fix this?

[jbee]

Implementations are using it to be able to perform the export

It can be used but since I know it is extra work I use getNames() and getMetrics(String) and getMetadata(String) which is not guaranteed to be a snapshot of the full registry but then there is no such requirement. It produces a consistent view in the sense that each group with the same name is consistent and that at the time of the export these were the groups existing. Even with the maps you don't have any better guarantees as you needs the Metadata and the Metric objects. You cannot receive both in a single operation so there is no actual snapshot consistency. And even if there would be such an operation it is unlikely to be implemented in a "stop the world" fashion. The implementation would basically use the same level of consistency assembling a result by iterating their internal data in a way that allows it to change while doing that. So consistency and snapshots aren't really arguments here I feel.

[jmartisk]

Ok, I agree we currently don't have a requirement for the export to be a fully consistent snapshot, and we probably don't want to introduce such requirement, because of performance reasons. It might not be a necessity to have under all situations.

But if an implementation still decides that they want to do fully consistent snapshots, they will probably want to use getMetrics() and getMetadata(), both together under a lock or something, to make sure both of them return consistent data.

[jbee]

lock or something, to make sure both of them return consistent data.

For that to work the lock would need to be within the registry so it can block all modifying operations attempted during the export. If you go to such a length to create consistent snapshots you really would not do that and still require correct use of locking an multiple methods to create such a snapshot - instead a export method accepting some kind of writer would be added to the registry that ensures the consistent writing.

[jmartisk]

instead a export method accepting some kind of writer would be added to the registry that ensures the consistent writing.

That wouldn't work very well if you have multiple exporters with different formats.. I think the best way to implement this thing would be that the metric registry implementation class adds its own method that returns some "snapshot" object (implementation specific) and does it with proper locking, and it will potentially include calls to getMetadata and getMetrics. That would be left to the implementation, and I think we shouldn't tell them to not use getMetadata and getMetrics for that.

But even if full consistency is not required, calls to getMetadata and getMetrics (without synchronizing them together) are a valid way to go, and easier than combining getNames() with getMetrics(String) and getMetadata(String).

[rmannibucau]

@jbee getNames() + getmetric(string) is NOT a replacement since getmetric can return null while in getNames() (keep in mind the registry is dynamic at runtime). The plural methods had the guarantee it both exists or is not present which is a super important features to keep user code simple and avoid to have to handle in user code all that dynamism. Since all implementations have a concurrent map (like) storage, the mapping from the internal to plural (getMetrics()) model is trivial and simplifies a lot user code. IMHO it is key for the spec adoption to keep it simple as it was.

[jbee]

As a user you certainly have to write different code to do the same thing compared to using a map and without question a map is more convenient to use if you want to process all entries. The issue is that the map was the the more convenient way to resolve single metrics by MetricID so it got used for that too. With the new methods added to the registry this now can be avoided and the deprecation should help to identify this usage pattern and avoid doing it in the future specifically because it is so convenient and because the getter name suggests a "cheap" operation.

There might be better ways to communicate this but I cannot think of one that does not involve deprecating and later removing the problematic methods. The change wasn't about not offering a map but about avoiding it being used where it should not. So from my point of view the methods could also be renamed instead of removed later on to use a name that leaves no doubt that using the map methods is an "expensive" operation.

[rmannibucau]

@jbee I fully agree the getX(String) methods are needed but I also guarantee you the getXs() methods are needed as an user. In that regards, is adding more javadoc to warn about the usage instead of @Deprecated (which appear in the IDE and is reported by auditing tools) a solution to that ticket?

[jbee]

is adding more javadoc to warn about the usage

It certainly is an alternative way.

I considered the warning generated by deprecation a good thing. We want people to check the usage and maybe use other methods where appropriate. If this is considered going too far extended comment is probably the way to go. Although personally I find it unlikely that users will read any extended javadoc for a method that already existed and that they used for quite a while.

[rmannibucau]

I considered the warning generated by deprecation a good thing.

Deprecated means "you must not use and use something else", this is not the case here so it is not adapted IMHO.

[jbee]

Deprecated means "you must not use and use something else", this is not the case here

Use something else is what I did intent when doing the change. The idea was that the method would be removed in a later release. As you say there are valid uses for map but when I looked into it back then I found that they can be substituted with a combination of other methods as suggested by the javadoc I added.

If a method returning a map should stay I suggest adding an alias method and refer to that for actual use as map and later remove the methods in question. If the methods are plain removed in a later release I think deprecating them now is the best way. If neither an alias method should be added nor the methods be removed deprecation should be removed and additional javadoc should suggest using map only for map processing cases.

That said let me also point out that the map processing also needs null checks. If you iterate map of metrics you need to null check the metadata, or other way around if metadata is the outer iteration the metrics maps result needs null checking. I cannot see how the two maps actually improve the situation enough over getNames() combined with getMetadata(String) and getMetrics(String) also given that doing the processing e.g. for an export might be used a few times only. As your own comment suggest using the maps might trick you into thinking no null check is needed which is not correct and can cause NPE.

[rmannibucau]

I found that they can be substituted with a combination of other methods as suggested by the javadoc I added.

I disagree on that, there is no replacement in current API and this lacks (even in the milestone).

That said let me also point out that the map processing also needs null checks.

This is not true since most usage is to process the map and not do N+1 queries (you can check out exporter needs for example). This is why there are these 2 map accessors. If you aim at adding a method doing the resolution of the 3 entities, I can agree it is better - but we would need to discuss why we have 2 entities since it is not needed and was likely an abuse of v2. That said the 2 map getters are perfectly enough for most cases IMHO.

[jbee]

You used the export as an example where maps would be used. If you do that you need both metrics and metadata maps. These two are not guaranteed to be consistent to each other so you do need to null check as in the time between accessing the two maps the data basis could have changed. This is the same issue you have when using the methods I suggest to use.

[rmannibucau]

@jbee strictly speaking you don't need both for most cases outside the spec.

[donbourne]

+1 on javadoc approach. There's a reasonable case to be made for exporters wanting to use the getMetrics() method, so if the javadoc for getMetrics stated that you should use the other get methods if you just want a single metric, I think that would be sufficient.

[jmartisk]

So to my understanding we will undeprecate the methods, clarify the javadoc and clarify that these two methods return a copy (snapshot) of the current state. @rmannibucau care to send a PR? :)

[jbee]

these two methods return a copy (snapshot)

Copy might not be the right wording. It might be a computed collection or a copy - that depends on how it is implemented. The spec maybe should state that this is decoupled from any changes applied to the state of the registry after calling the method and that changes to the registry while calling the method and computing or copying the result might affect the result meaning that the result is consistent but it might not contain elements removed after calling the method but before it returning to the caller or likewise might contain elements added after calling the method but before it returning to the caller.

[rmannibucau]

oki, will send a PR trying to find a "not too bad" wording ;)

[rmannibucau]

here it is https://github.com/eclipse/microprofile-metrics/pull/575