eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

Clean up any small problems in TCK #718

Closed Channyboy closed 1 year ago

Channyboy commented 1 year ago
- Ensure metrics that use the same name x tag combination are exactly
the same (i.e. can't have metrics with same name where one uses tags and
the other does not, or that all tag keys used are exactly the same
- remove references remaining references to removed metrics meter (i.e.
meter)

Fixes #714

May cause conflicts if https://github.com/eclipse/microprofile-metrics/pull/716 is merged first. Will need to rebase then.

tjquinno commented 1 year ago

I apologize if we discussed this and I've forgotten.

IIUC this imposes a new restriction that are different from prior releases: Metrics with the same name must also share the same tag name set.

In particular, this prohibits two metrics with the same name and one with no tags and the other with some tags.

Can someone refresh my memory as to the reason behind adding the restriction?

Channyboy commented 1 year ago

@tjquinno This was mentioned long time ago in the meeting minutes or alluded to this issue.

Prometheus output seems to dictate that any multi-dimensional metric is done so in with the same name and same tags.

https://prometheus.io/docs/instrumenting/writing_clientlibs/#labels

"Metrics from custom collectors should almost always have consistent label names. As there are still rare but valid use cases where this is not the case, client libraries should not verify this."

The quoted text does mention that mixing of labels is possible. But the exporter in the Prometheus formatting library (i.e. prometheus' common java client) strictly follows the consistent name and label portion and will just provide the first metric x label combo metric registered under that name and ignore everything else.

jgallimore commented 1 year ago

@Channyboy Would this Metrics with the same name must also share the same tag name set. affect this test: org.eclipse.microprofile.metrics.tck.MetricIDTest#removalTest? This test seems to suggest that different metrics could have the same name, but different tags, and that removing a metric from a registry by just the name would remove all the metrics with that name (even with the different tags).

I'm ok with enforcing this statement: Metrics with the same name must also share the same tag name set, but if we want to do that, we should change this test to enforce that - perhaps requiring implementations to throw an exception if an attempt is made to add metrics with the same name but different tags to a registry?

tjquinno commented 1 year ago

I think @jgallimore has it right.

If the spec mandates an invariant that all metrics with the same name have the same tag name set, then implementations must enforce the invariant when the app attempts to register a metric by throwing perhaps an IllegalArgumentException?

Channyboy commented 1 year ago

@jgallimore @tjquinno Created Issue at : https://github.com/eclipse/microprofile-metrics/issues/721