Metadata/MetricsID link?

[rmannibucau]

Registry returns a Map of String/Metada and MetricsID/Metric, this looks unlikely and not symmetric. Is it intended?

[jmartisk]

Are you referring to methods Map<MetricID, Metric> getMetrics() and Map<String, Metadata> getMetadata()?

If so, then yes it is intended, the <String, Metadata> map is from a metric name to the relevant metadata object, because given a metric name, you can't retrieve a Metric directly because there can be many of them under that name. But they all are required to share the same metadata, so getting the metadata is possible. To get one specific metric, you need to pass the MetricID, for which the first method is.

We could potentially introduce Map<String, List<Metric>> getMetricsGroupedByName() or something, if people wanted, but it feels overly complicated and one can compute this map programmatically by calling getMetrics() and then processing it using the Java Streams API or similar.

[rmannibucau]

@jmartisk this is not consistent for me, it should me Map<MetricID, Metadata> getMetadata() because by contract MetricID is the only identifier and metadata is attached to a Metric so the sharing is inaccurate - I understand it as being a missing step of the MetricID addition in the spec. So I think a 2.x should be done adding the right getMetadata() and deprecated the String as key method. Endpoint should also likely get the optional tag (query) parameter to be able to select a particular metric/metadata/.... The common needs are to map an id to a metric but also to map an id to a metadata and the join relationship: a metadata to a metric.

Hope it makes sense

[jmartisk]

Oh I understand now, so the issue is that the map is not keyed on the "primary key", which is MetricID. I agree this might look a bit confusing, but on the other hand, one should in this case consider the name as the "primary key" (there is a 1:N relationship between name and metadata, not between metricID and metadata), and so if we also had a Map<MetricID, Metadata>, then this map would potentially contain a lot of duplication, because multiple metrics with the same name (but different MetricID) will always have the same metadata and will increase the number of entries in the map without much benefit. Personally I'm not sure if fixing this minor mismatch is worth introducing a map that will be bigger, with all its memory and CPU usage penalties, especially if the original map contains all necessary data. For obtaining Metadata of a metric, you can now simply call registry.getMetadata().get(metricID.getName()) instead of registry.getMetadata().get(metricID)

[rmannibucau]

Guess design issue is deeper: what's the point of metadata and metricid? One should disappear imho. The mem overhead is very light, generally 0 (changing tags but not names is uncommon) and when done it is a pointer and its node wrapper so almost nothing. Im clearly +1 to rollback to v1 design which was way more meaningful for me and as powerful as current one for all apps.

[jmartisk]

Version 1 design didn't allow to have multiple metrics with the same name differentiated by tags. Tags were in a 1:1 relationship with a metric, so they were basically useless for anything else than differentiating metrics coming from different service instances (and Prometheus itself can do the same thing anyway, it can append a tag automatically based on the source where it read the metric from). So I'd need you to elaborate on what exactly from version 1 to restore, because there the tags were quite useless compared to the current model.

The point now is that you can have multiple variants of the same metric, tagged with various tags, and it makes sense that they need to have the same metadata, otherwise we wouldn't be able to produce a reasonable export for Prometheus, which uses pretty much the same concept of tags. That's also the reason why metadata map is keyed by name, not by id.

If you have a proposal how to sanitize the possible confusion with name/metadata/metricid without limiting powerfulness of the model (and preferably without major compatibility breakage), we'll be glad to hear it, but version 2 is clearly more powerful than 1.

[rmannibucau]

Ok, my idea was to have the id to stay the name, forbid two metrics with the same name and each name has a metadata (with tags) and metric instance. This is simple, 99.99% of the usage and avoids an unfriendly user model IMHO.

[donbourne]

The problem with having tags in the metadata would be that each time I want a new tag-variant of a metric I would have to specify some parts of the Metadata (the non-tag parts) being sure they exactly match the values in the other tag-variants, while other parts of the Metadata (the parts that specify the tags) would change. Expecting people to get that right seemed problematic. By making the Metadata constant for all tag variants of the same metric name it avoids this problem and hopefully results in less "user-error".

[rmannibucau]

Technically it should be close to a metadatabuilder.of(sourcemetadata) (not the right syntax but idea is we have a copy builder).

Now, is duplicating metrics without metadata accurate? Does it make dashboards readable and useful? Does it match real life deployment and coding? I dont think so and I would even expect to be able to pass tag on all endpoints to filter the exact data i want, even for metadata.

If your last point is true it means we shouldnt be able to register 2 gauges or 2 counters which obviously is wrong but would be simpler since we would merge all data in one, this is why i think v2 was an error and should be simplified and cleaned up.

[donbourne]

Now, is duplicating metrics without metadata accurate?

We make that easy by not even requiring you to specify the Metadata if you already have registered a metric with the same name that you just want a new tag instance for:

metricRegistry.counter("someName", new Tag("world","earth"));

Does it make dashboards readable and useful? Does it match real life deployment and coding?

I'm not sure what you mean here. The dashboards for Grafana don't rely on the metadata -- the metadata is mostly useful for humans looking at the /metrics output.

I dont think so and I would even expect to be able to pass tag on all endpoints to filter the exact data i want, even for metadata.

Using PromQL you can easily query for your tagged version of each metric. eg. I can query for someMetric{someTag=someValue}. I'm not sure I'm getting your point on that correctly.

[rmannibucau]

We make that easy by not even requiring you to specify the Metadata if you already have registered a metric with the same name that you just want a new tag instance for:

Which is also seen as a bug cause you can inherit metadata you dont want.

About dashboards: when you build a UI on metrics of the spec you use the meta to show the units, show the display name etc... Here, I will do the same comment than on some other MP specs: there is not only 1 k8s mainstream solution to write apps and k8s ecosystem being the most recent is also the least mature so we should enable people to migrate apps on more modern stacks without requiring all the infra to change at once too and, even if specs are not that bad generally, such choices are slowly killing these usages and make MP a pure k8s solution which is IMHO a very bad choice, k8s is just one deployer.

Using PromQL you can easily query for your tagged version...

Here again, prometheus is not the best solution today nor the most used one even if often coupled to k8s when there is no real idea of what is needed, not assuming you have an expressive language outside simple regex would be saner IMHO. It is also a structural change from 1 to 2 without any way to maintain existing apps in prod - breaking an API is one thing which is visible at compile time, breaking prod deployment is way harder to harness and way more impacting and $$.

[ebullient]

This feels like an implementation detail that shouldn't be in the API at all. A) the MetricID (Name + tags) maps to a single registered meter B) If I supply additional metadata, an implementation could store that metadata as part of the meter (for export purposes)

This feels like a case of things being overspecified. What an implementation does to store or use metadata shouldn't be exposed via the API, should it? When is it useful to look up metadata?