search

Azure / diagnostics-correlation

Microsoft Diagnostics Correlation
MIT License
15 stars 8 forks source link

Feedback on correlaiton design document #1

Open SergeyKanzhelev opened 7 years ago

SergeyKanzhelev commented 7 years ago
  1. Taxonomy of correlation id and request id doesn’t match application insights’s toxonomy. Why introduce new term “correlation-id”. Either use root Id as we do in AI or traceId as opentracing uses
  2. Do we support “nested” spans? Will they define “requestId” or “spanId”. Taxonomy is not quite clear
  3. “filled by the library” – is it true? Is it generated by the framework? Like IIS’s request ID? Can I fill it from my object I extracted from the queue? Or some other source?
  4. Can there be more than one context factory? How do they know about each other? Which context factory stores the context when I ask it to do it – the first or whoever can store it? Like if I have System.Web.HttpRequest and OWIN context facotires together – which one will save the context for me? Or I should decide it myself?
  5. Where AsyncLocal came from in the “Request flow” picture? Is there the default Context provider that is using AsyncLocal?
  6. Do we support other concepts of OpenTracing in the correlation library? Like spans with timing information and name of the span/spancontext?
lmolkova commented 7 years ago
  1. Correlation works with the same set of ids as AI, the names are different: correlation-Id is well known term and I would say that root-id may be misleading. Opentracing standard does not cover which ids should be used, Trace_id and span_Id are part of Dapper or Zipkin specs.
  2. Correlation lib now does not have Spans or any kind of Span. It only has context. Regarding the nested operations: correlation in some way supports nested context for the outgoing requests:
    • if user decides to use HttpClient DelegatingHandler, library assumes user also has handler to log the request. Logging handler is responsible to get nested context from the request (lib provides API for it)
    • if user wants to use DiagnosticSource (or profiler) instrumentation library provides notifier with nested context to notify about the outgoing request events.
    • nested contexts are not stored in AsyncLocal
  3. CorrelationContext must be created with correlation Id, it's extracted from the request headers. If incoming request does not have header, request trace identifier is used (which is probably IIS request id). You can create a context with any correlationId
  4. Currently no: there could be just 1 context factory. If we allow more (which was the plan), they don't have to know about each other. They could independently extract their pieces of context and those pieces should be merged together in one context. The IContextFactory API should change accordingly, to return KV pairs instead of context. Correlation lib should provide API to call all factories and merge context
  5. There is ContextResolver class wrapping AsyncLocal variable and providing access to it.
  6. We do not support anything from Opentracing. I think one of the results of this review would be to extract common pieces of distributed tracing and decide if they will be opentracing or ILogger implementation or anything else