Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Add tag based constructors for simple metrics. #1685

Closed
wants to merge 4 commits into from

Conversation

fitzoh
Copy link
Contributor

@fitzoh fitzoh commented Feb 24, 2020

While attempting to add partition tags to metrics in #1680, I discovered that the existing stats library does not work with tags, as the suffix is always tacked on the end of the metric, leaving us with stats like sample.metric;key=value.gauge32 (as opposed to sample.metric.gauge32;key=value or sample.metric;key=value;type=gauge32).

This PR adds a NewxxxWithTags() function for gauges and counters, adding a type tag to those metrics instead of the current .<type> suffix.

Metrics other than counters and gauges have more complicated suffixes (.min, .max), so I'm punting for those at the moment and only addressing the trivial metrics.

Initial commits migrated Gauge64 to use a struct like all other metrics do and move the extra x that precedes all suffixes out of the writer method so that it isn't automatically added when using tags. You'll probably want to review this commit by commit.

@Dieterbe
Copy link
Contributor

some interesting backstory for you : #1294

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

Nice, excited to have tagged MT metrics in the future

@fitzoh
Copy link
Contributor Author

fitzoh commented Feb 25, 2020

Appreciated @replay , holding off on merging to give @Dieterbe a chance to poke around.

Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

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

see my commit above.

also, as discussed in #1294 I don't think a "metric type" tag is useful (neither is stat for that matter), because aggregations across different values for these tags are not meaningful.
in our case, for any given metric key, we'd have only 1 type tag, so we wouldn't have the issue of "non-meaningful aggregations" but it seems silly for all metrics to have 1 type tag with only 1 value. I like the approach pushed forward by prometheus of making tags useful for aggregations.

we could however append the type to the end of the metric name, before adding tags, but also as pointed out in #1294 they should be more specific and account for signed vs unsigned integers

@fitzoh
Copy link
Contributor Author

fitzoh commented Feb 27, 2020

Gonna close this in favor of #1696 for the moment, which should support @Dieterbe 's suggestion regarding metric types

also, as discussed in #1294 I don't think a "metric type" tag is useful (neither is stat for that matter), because aggregations across different values for these tags are not meaningful.
in our case, for any given metric key, we'd have only 1 type tag, so we wouldn't have the issue of "non-meaningful aggregations" but it seems silly for all metrics to have 1 type tag with only 1 value. I like the approach pushed forward by prometheus of making tags useful for aggregations.

regarding

we could however append the type to the end of the metric name, before adding tags, but also as pointed out in #1294 they should be more specific and account for signed vs unsigned integers

This would makes sense, but should probably be done as a standalone change.

@fitzoh fitzoh closed this Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants