Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unregister the metric fully when last labelled metric is unregistered #110

Merged

Conversation

simonjbeaumont
Copy link
Contributor

Motivation

The registry is designed to return the same metric instance for a previously registered metric with the same name if, and only if:

  • the metric type matches;
  • the metric label keys match (it's fine for the labels to differ); and
  • in the case of histograms, the buckets match.

By design, it throws a fatal error if these conditions do not hold for the metric name.

However, the registry also provides methods for unregistering metrics, such that they will not be emitted. A consequence of this unregistration is it allows for the metric name to be reused:

registry.unregisterCounter(registry.makeCounter(name: "name"))
_ = registry.makeGauge(name: "name") // does not crash

But the API has some unexpected behaviour when unregistering metrics that were registered with labels:

registry.unregisterCounter(registry.makeCounter(name: "name", labels: [("a", "1")]))
_ = registry.makeCounter(name: "name", labels: [("b", "1")])  // crashes

This is because the unregister APIs currently only remove the dimensions but do not go as far as to unregister the whole metric once the last such dimension has been unregistered.

Modifications

  • Unregister the metric fully when last labelled metric is unregistered
  • Add test, testUnregisterReregisterWithoutLabels, which passed before this patch.
  • Add test, testUnregisterReregisterWithLabels, which failed before this patch, but now passes.

Result

Unregistering a metric with a label will unregister the metric name if this was the only remaining dimension for that metric.

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix! Thank you!

@fabianfett
Copy link
Member

@swift-server-bot test this please

@fabianfett
Copy link
Member

@swift-server-bot test this please

1 similar comment
@fabianfett
Copy link
Member

@swift-server-bot test this please

@fabianfett
Copy link
Member

@swift-server-bot test this please

Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@ktoso ktoso merged commit 785478c into swift-server:main Feb 26, 2024
5 checks passed
@ktoso ktoso added this to the 1.0.3 milestone Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants