search

spandex-project / spandex

A platform agnostic tracing library
MIT License
335 stars 53 forks source link

Add `traced/1` decorator #8

Closed zachdaniel closed 7 years ago

zachdaniel commented 7 years ago

Our documentation is currently incorrect, as it claims that a traced/1 decorator exists that would allow for adding context to the parent span.

driv3r commented 7 years ago

This topic creates actually another question, why do we even need to distinguish between trace & span at the moment? Why just not have always a span, and create a parent/trace when we are missing it? I think this was actually the original implementation.

At the moment from what I saw we only log error, instead of creating span instead.

zachdaniel commented 7 years ago

You might be right about that.

Not necessarily disagreeing, but some more food for thought: The top level span is special as far as dd is concerned. For instance, it is the only one that needs to have the http metadata, and whether or not it is an error is what dictates whether or not the whole trace is an error. Other implementations (I think this is how Google/Stackdriver tracing is set up) have the trace as a discreet entity that contains spans, so having an explicit entry point for starting a trace would be better for extending to other adapters. Also, imagine that someone forgot to start a span around the primary call that they want to trace, and that thing calls 100 functions sequentially, all of which are spanned. If we do it for them, then they will get 100 traces, one for each thing they intended to be a span, which isn't really what they wanted.

zachdaniel commented 7 years ago

I'll take care of this issue tomorrow or the day after. We can upgrade the decorator library that we use to also really clean these up, as they now support default arguments in decorators.