taoensso / telemere

Structured telemetry library for Clojure/Script
https://www.taoensso.com/telemere
Eclipse Public License 1.0
199 stars 5 forks source link

add signal->span-attrs-fn option to opentelemetry handler #28

Closed benalbrecht closed 1 month ago

benalbrecht commented 1 month ago

this PR adds a :signal->span-attrs-fn option to the opentelemetry handler for adding custom signal attributes to the corresponding opentelemetry span.

the existing signal->attrs fn seems to be intended for log records and i'm not sure whether the result would be a good default for spans as well, so i've just made it public to have it available as an option.

i've made the helper functions public as well to aid in creating a custom signal->span-attrs-fn

ptaoussanis commented 1 month ago

Thanks for this Benjamin!

I'd like to understand your use-case a little better. Could you please give me some idea of what one would typically use signal->span-attrs-fn for?

What would your fn look like?

Also, just checking if you're familiar with the :otel/attrs feature (currently undocumented).

benalbrecht commented 1 month ago

my use-case is adding attributes to otel traces i'm sending to honeycomb.io.

my signal->span-attrs-fn would look similar to signal->attrs

yes, i've noticed :otel/attrs but they aren't being added to trace spans right now 😄 just adding :otel/attrs might also work for me, since they could be populated by middleware.

ptaoussanis commented 1 month ago

my use-case is adding attributes to otel traces i'm sending to honeycomb.io. my signal->span-attrs-fn would look similar to signal->attrs

Could you be a little more specific? E.g. are the attributes common to all signals, or signal specific?

Can you give me an example of just a few of the attributes you plan to add? It'd be helpful to get an understanding of what problem you're actually trying to solve, and therefore what our best options might be.

i've noticed :otel/attrs but they aren't being added to trace spans right now 😄 just adding :otel/attrs might also work for me, since they could be populated by middleware.

Just to confirm: you want your custom attributes on the trace and not the log record?

What if the handler supported the following?:

Or if we stick to your idea of an signal->span-attrs-fn option, how would you feel if it took a map like this?

In principle I'd prefer to avoid making low-levels fns public unless really necessary. We may still want to make adjustments later.

benalbrecht commented 1 month ago

Could you be a little more specific? E.g. are the attributes common to all signals, or signal specific? Can you give me an example of just a few of the attributes you plan to add? It'd be helpful to get an understanding of what problem you're actually trying to solve, and therefore what our best options might be.

Some attributes are common to all signals, some are singal specific. An example:

What if the handler supported the following?: :otel/log-attrs -> handled as here. :otel/trace-attrs -> handled the same way, but for trace attributes :otel/attrs -> attached to both trace and log

i'm not using the otel log handler, so i don't have a preference here 😄 a single :otel/attrs key that is applied to both would work for me.

Or if we stick to your idea of an signal->span-attrs-fn option, how would you feel if it took a map like this? In principle I'd prefer to avoid making low-levels fns public unless really necessary. We may still want to make adjustments later.

using something like :otel/attrs seems to be the better approach. i don't see anything that this version of signal->span-attrs-fn could do that can't be handled with middleware as well.

ptaoussanis commented 1 month ago

Great, thanks for the extra info 👍 In principle how does this look to you?

Also, we could hypothetically add the same custom attributes here, for Span Events.

Any thoughts on that?

benalbrecht commented 1 month ago

looks good, thanks! it would make sense to add the attributes to span events as well, I totally missed the extra handling for those.

ptaoussanis commented 1 month ago

it would make sense to add the attributes to span events as well,

Great, makes sense. I've updated the commit.

If you have no other suggested improvements, I'll document the :otel/ options tomorrow and clean up the commit 👍

benalbrecht commented 1 month ago

i'm closing this one in the meantime :) looking forward to the next release!

ptaoussanis commented 1 month ago

I just updated v1.0.0-SNAPSHOT on Clojars.

If you get an opportunity, would you please test this and confirm that it's working as expected?

In that case, I expect to be able to release RC1 before the end of this month (Oct).

Cheers!