allow default tags

[connorjacobsen]

I wrote this for myself, and I don't know if you want something like this in the library or if other people would find it useful, but I figured I would give you the option either way.

I keep finding myself wanting to easily include information like app environment with every metric, and I don't particularly want to be making a System.get_env call or storing it in an agent (or ets), and want it quickly available each time a metric gets published. This seemed like a reasonable way to do that.

If you might want to include this and want/need edits, I'm happy to make those. If you don't want this, feel free to close the PR.

Thanks for the library, saved me a bunch of time :)

[connorjacobsen]

@arkgil After thinking about this a bit more (and realizing that my current implementation doesn't allow the globals to be overriden), I have a desired behavior question for you. Should global_tags get sent with every metric, or just the ones that have specified they should include a tag from the global_tags set?

Specifically, if I start the module with global_tags = [env: "test"], create a metric that looks something like: Telemetry.Metrics.summary([:foo, :bar, :baz], tags: [:method, :status]), and then emit a metric with :telemetry.execute([:foo, :bar, :baz], 123, %{method: "GET", status: 200}), should the env tag be included or not? In my current implementation it is included, but I'm wondering if it should need to be required when the metric is defined. Thoughts?

[arkgil]

@connorjacobsen good question!

Since they are called "global" or "default", I'd assume that they are published with every metric.

On the other hand, making it configurable gives us explicitness and an option to opt-in whenever we feel like we need a tag. I think that this is the way to go. What do you think?

[connorjacobsen]

I think I'm a fan of being explicit. It would be less surprising and allowing users to opt-in on a per metric basis seems kinda nice.

[arkgil]

@connorjacobsen awesome, let's go with this approach, then!

[connorjacobsen]

@arkgil Pushed some updates. Let me know what you think about this implementation.

[connorjacobsen]

Should we try re-running that one to make sure it passes? I don't think I did anything special so it should be fine?

[arkgil]

@connorjacobsen I re-run it several times already, and it still seems to fail randomly. Let's merge, and I'll check this failure locally.

[arkgil]

@connorjacobsen I've been thinking about the decision we made about requiring the metric to opt-in to using global tags. While I agree that this behaviour is clearer, I think that it violates the spec of Telemetry.Metrics:

Tag values are fetched from event metadata (...)

That means that whatever we set as metric :tags, should either come directly from event metadata or from metadata transformed with :tag_values function. What we have now doesn't fit that behaviour. Considering this, maybe we should just include global tags in every metric? What do you think?

[connorjacobsen]

It seems like if we really want to stay true to that to the letter, we would probably need to remove this feature entirely. If we simply included all global tags regardless, we are still manipulating the tags without using the event metadata or the tag values function.

Though just including them on every metric would probably be more similar to how the library currently adds the prefix to the metric before reporting it. It probably just comes down to how you want to trade off behaviors vs adherence to the spec as the library owner. Happy to do whichever you prefer.

[arkgil]

@connorjacobsen let's keep it the way you did it. Thanks again for the PR! 😊

[iautom8things]

Thank you for this feature, I think y'all did a great job honing in on the right api for it. Could we please get an official hex release that includes this feature?

[arkgil]

Yes, sorry for the slack, I'll cut the release ASAP!

[arkgil]

@iautom8things @connorjacobsen 0.3.0 is available on Hex :)

[iautom8things]

Thank you! 🎉 ❤️