Added span/3 function

[akoutmos]

After reading through #57, #56 and the various PRs to Finch, Redix, etc, I figured it might be a good idea to capture all that information and add it to the docs. Thoughts?

[josevalim]

Docs are welcome, thank you! At the same time, I think it may be best to go ahead and add a telemetry:span/3 as originally proposed in #57. Would you like to send a PR for that? :)

[akoutmos]

Sure thing. I can probably knock out the telemetry:span/3 call over the weekend. Thanks for the quick response!

[akoutmos]

That was a lot quicker to implement than I thought it would be :D. span/3+docs+tests have been added. Ready for review. Thanks in advance for any feedback!

[josevalim]

@bryannaegele I don't think it should be a macro, at least not initially. We should do our best to provide functional APIs and then build any macro convenience on top of it.

Regarding the use of this function, I can see many cases where we have implemented span by hand - and made mistakes - where we could have used this function instead.

[bryannaegele]

@josevalim I agree on both points and there is a need.

My concern is that with a macro we have the ability to add the features we need for auto-discoverability and requirements on the user in the case of having to specially wrap the return value of your function. The latter may be unavoidable in the case of needing to filter the metadata.

How does this to look in a real-world example? The docs and tests don't include one. We should have that.

[akoutmos]

I'm coming into this a bit late so perhaps I am missing some context (apologies).

Could you expand upon this bit "The latter may be unavoidable in the case of needing to filter the metadata." @bryannaegele ?

Good point on real-world example. I'll add something to the README showing this can be used.

[josevalim]

@bryannaegele the one we had in Plug and the ones in Finch can definitely be rewritten using this function: https://github.com/keathley/finch/blob/master/lib/finch/http1/conn.ex#L31

Someone is welcome to rewrite those as an exercise. :)

[bryannaegele]

Could you expand upon this bit "The latter may be unavoidable in the case of needing to filter the metadata." @bryannaegele ?

We may have to leave this to the user. If we use this for tracing or logging, it's frequently necessary to filter what you're shipping to logs or traces for sensitive data or for size. It would be more efficient to do this at the point of the call versus trying to filter an entire stream.

[akoutmos]

Could you expand upon this bit "The latter may be unavoidable in the case of needing to filter the metadata." @bryannaegele ?

We may have to leave this to the user. If we use this for tracing or logging, it's frequently necessary to filter what you're shipping to logs or traces for sensitive data or for size. It would be more efficient to do this at the point of the call versus trying to filter an entire stream.

Ah I see what you are saying. In my opinion it would be best to put that responsibility on the user as you mention. I imagine that the number of "event filter options" that people could come up with could be quite large and that may not be something to put into core telemetry. I remember making a PR to Phoenix that was (rightly) rejected to do something similar. Filter telemetry events based on request path. That led me to writing https://github.com/akoutmos/unplug which I think solved my problem and many other problems. Just my .02 :)

[akoutmos]

@bryannaegele Sample usage of telemetry:span/3 in both Elixir and Erlang has been added to the README :)

[bryannaegele]

Should start_metadata be required for users to populate? It feels like that should be an optional third argument, so we'd have telemetry:span/2 and telemetry:span/3 with the third arg being that optional metadata.

[sorentwo]

Should start_metadata be required for users to populate? It feels like that should be an optional third argument, so we'd have telemetry:span/2 and telemetry:span/3 with the third arg being that optional metadata.

That's the approach we took for oban. Very few of the calls to span actually provide metadata, so having it as a third optional argument feels cleaner in most cases.

[josevalim]

I don't mind forcing users to add metadata. If they can do so, it is usually better, it enriches the events. If they really don't have anything to pass, they can be explicit about it with an empty map.

[bryannaegele]

I don't mind forcing users to add metadata. If they can do so, it is usually better, it enriches the events. If they really don't have anything to pass, they can be explicit about it with an empty map.

I agree on the general principle that more available information is better than less, but in practice this particular metadata is just for the start and exception events which is rarely used. We're still providing an empty map in the case of it being optional, so it's still a conscious choice to go that route without having to write it out.

Where I have implemented it for the exception or start events, I've just made it a map of the arguments. Have you added anything else in your implementations @josevalim? I can certainly be convinced that we want that as the default from the user, but we should explicitly provide that as guidance in the docs - "This should at least be a map of the arguments to your function".

[akoutmos]

After thinking about the optional metadata stuff a bit, I think I am leaning towards being explicit and having the user specify the empty metadata map. Sort of how it is done with attach/4 + attach_many/4 where in most cases I don't see people leveraging the Config argument and just set it to nil.

On the other hand it seems like we also have a bit of a style discrepancy as execute/2 is a convenience function execute/3 but with an empty map default.

All this to say that once we pick a style of approaching this, we should propagate that style to the other functions. I.e either remove execute/2 or add a attach/3 + attach_many/3. The latter would probably be an easier route to go as it would not be a breaking change.

Just my 2 cents on the issue at hand.

[josevalim]

@bryannaegele please let me know if you are happy with the current iteration so we can ship it. :)

[bryannaegele]

@josevalim I'd still like to see the args reordered to make the metadata the last argument to at least allow for a shorthand version even if we don't add it now. What are your thoughts?

[josevalim]

I have mentioned above that I would prefer to be explicit about the metadata. Most events do have metadata and if there is no metadata I don't mind asking users to pass an empty map. I wouldn't worry about a shortcut API unless there is evidence we need one. I would prefer to ship a minimal API and build on top of it than try to figure it all out upfront.

[bryannaegele]

I heard you loud and clear :) I wasn't pressing to add the shortcut. The question about term ordering was that it provides flexibility with the metadata as the last argument if we swap it unless you feel the API is awkward with that shape.

[josevalim]

The anonymous function is typically the last argument in Elixir, so I would stick with that to keep the API idiomatic there too.

[josevalim]

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: