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

Metric-level tags #1

Closed
ysamlan opened this issue Apr 5, 2019 · 0 comments
Closed

Metric-level tags #1

ysamlan opened this issue Apr 5, 2019 · 0 comments
Labels
enhancement New feature or request

Comments

@ysamlan
Copy link
Owner

ysamlan commented Apr 5, 2019

Quick braindump on this.

In the current implementation, tags are attached to the entire batch only; you can't selectively put tags on measurement/metrics individually. That's mostly a limitation of the underlying go-metrics library / Dropwizard itself; measurements don't have any concept of "metadata."

Options:

Store the metric metadata inside this library:

We could accept an extra param map[string]map[string]string (ugh) mapping individual metric names to metric-specific tags and stash it in our struct, then attach those tags to the metrics that match that name before uploading them. Simple to implement, but then those tags would apply to all measurements with that name, which seems not super useful.

Store the metadata in the metric name & wrap access to the metric:

Follow the model of https://github.com/appoptics/metrics-appoptics#usage and provide helpers that encode the extra tag data into the underlying go-metrics metric name (in Java they're doing that via full JSON serialization so the underlying dropwizard metric name becomes something like {"name":"metricname", "tags":[{"name":"host", "value":"localhost"}],"overrideTags":false} - which seems kinda overkill and in their case involves adding a whole separate metric name caching layer.

We could maybe do the name-based storage more simply using just joins/splits on characters that are disallowed in Appoptics metric names and tag k/vs anyways. Appoptics metric names must be [A-Za-z0-9.:-_], tag names/values [-.:_?\\\/\w ] - so something like metricname#env=prod,host=localhost would work fine, and it means we could just do a quick loop + string join (sorted on keys!) to get the names, and at posting time we'd just be splitting strings to build the tag map. Added advantage is that the names might look a little cleaner/saner when using other uploaders or viewers to those metrics. (Side note, we should enforce those tag name/value constraints the way appoptics does by replacement - if it's not prohibitively expensive to do at metric-tagging time we could do it there in one shot, otherwise we'd at least need a tagging-time enforcement on #=, even if the rest of the sanitization happens later).

Aside: I'd also probably opt to skip the Java lib's "override tags" option's complexity and go with the IMO saner option of just always starting with the global tags and merging in any per-metric tags (that is, always get global reporter tags; metric tags will always override them if present and conflicting). From what I can tell the Java library there does something weirder, which is start with the metric's tags, and then override those with the reporter's tags (effectively - it actually adds another Tag with the same name to the end of its internal tag list, then merges them at upload time with the later-arriving reporter value), unless you call doNotInheritTags on the reporter before creating the metric which is confusing!

Downside to name-based storage: The actual underlying metrics would be split out into (maybe less easily parsed?) names, which means they'd show as separate metrics if using another reporter (eg go-metrics expvar-style HTTP endpoints). It'd also mean the actual end-user's metric tagging becomes somewhat coupled to this specific reporting library (since the client's tagging is dependent on this reporter's helpers), but I think that's somewhat unavoidable if using a reporter-specific feature like this.

@ysamlan ysamlan added the enhancement New feature or request label Apr 5, 2019
@ysamlan ysamlan changed the title Grouped per-metric tags Metric-level tags Apr 6, 2019
@akahn akahn mentioned this issue Aug 23, 2019
@ysamlan ysamlan closed this as completed Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant