opentelemetry-erlang-contrib icon indicating copy to clipboard operation
opentelemetry-erlang-contrib copied to clipboard

Conveniences for Tasks

Open bryannaegele opened this issue 3 years ago • 3 comments

Starting a drop-in for Elixir Task module. This will allow users to just add an alias at the top of their file and progressively transition any tasks they want to trace.

I don't have a good solution yet for the mfa variations and I haven't tackled the supervisor variations.

bryannaegele avatar Apr 15 '22 22:04 bryannaegele

I’ve done nearly the same thing several times at work, so it would be nice to be able to have a maintained library implementation instead of copy/pasting between services. 😅

When I’ve done it though, I use the real names instead of _with_span etc so you literally just alias it in and it works the same as the stdlib module. I can help contribute what I’ve done as well for the parts you’re missing.

It does feel a little weird to do this, but IMO it’s easier that everyone having to always remember to manually put in 3 extra lines anywhere they want tracing to work across processes. There’s still the problem of remembering this alias so that they don’t get the stdlib version, but it’s easy to fix when you notice that your span is disconnected.

GregMefford avatar Apr 15 '22 23:04 GregMefford

In case you're curious then this is how we're doing it: https://github.com/salemove/opentelemetry_function . Basically task = Task.async(func) turns into task = Task.async(OpentelemetryFunction.wrap(func, "Some expensive calculation")). It can also be used with MFA.

We liked this approach because we can still continue using the original Task module and it's more explicit. It also can be used with other non-Task modules as well.

But I do see that some people may prefer not to use the wrap style syntax.

indrekj avatar Apr 18 '22 14:04 indrekj

@GregMefford I tried a couple of approaches just overriding the stblib functions but it just got messy and quite convoluted to cover the various uses (name, attributes, link/no-link, just ctx). I also think the API shouldn't override the default since that would affect all tasks in a module when you may only want to trace one.

For MFAs there will need to be some kind of wrapper like you have @indrekj because of supervisor spawned tasks. The only way to make it work that I can think of is to have the supervisor call this shim module which then applies on what was passed by the user, so that's helpful.

bryannaegele avatar Apr 19 '22 22:04 bryannaegele

One thing I'm going to try is just replacing the with_ctx calls altogether in favor of just replacing the base functions since I can't think of a reason to not default propagation.

bryannaegele avatar Aug 25 '22 14:08 bryannaegele

Yea, I'd agree removing that suffix makes sense.

tsloughter avatar Aug 25 '22 15:08 tsloughter