search

census-instrumentation / opencensus-csharp

Distributed tracing and stats collecting framework
https://opencensus.io
Apache License 2.0
139 stars 32 forks source link

Allow spans from the past #92

Closed SergeyKanzhelev closed 5 years ago

SergeyKanzhelev commented 5 years ago

As described in #92 - there are cases when you might need to report a span from the past. In that PR Redis library calls are recorded asynchronously. My guess why this API was chosen to expose Redis calls is extreme care of those calls latency.

Current API is quite limiting in this sense.

  1. ISpan doesn't allow to set start and end timestamps explicitly. Same for time events like annotations. Expanding ISpan interface may create some confusions.
  2. Alternative is to report SpanData directly. This approach can be problematic as a lot of logic like calling start/stop handler and sampling needs to be replicated manually.
  3. Another alternative is to either create custom Redis implementation of ISpan or make ISpan constructible from SpanData. I like this last approach. Not sure what may be the problems here.
SergeyKanzhelev commented 5 years ago

Couple more considerations:

  1. Allowing to override timestamps, messages and annotations makes the promise of consistency of those values (ordering and inclusion) fail. Keeping timestamps consistent is very important I think.
  2. OpenTracing API allows to pass end timestamp to span, but not the start timestamp.

Another scenario, besides the extreme care about the latency, where this approach may be useful - constructing spans from some third party library output or reading spans from file.

SergeyKanzhelev commented 5 years ago

After discussion we think that it is wise to avoid any timers inconsistencies in Start/Stop API. If span data needs to be exported for the span from the past - direct creation of SpanData should be used