Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    Tristan Sloughter
    @tsloughter
    re the correlation api, I had noticed none of the PRs used its terms but instead just SetValue, so I was also confused :)
    but makes sense to not get bogged down in shit like what terms to use, tags/hoplimit/etc, when trying to get the otep approved and then move to the spec
    I'm working on implementing this right now and automated in-process propagation/scope is where I'm partly stuck
    Tristan Sloughter
    @tsloughter
    and I just realized that the you answered my scope question but accidentally quoted it like it was still part of my question :)
    Ted Young
    @tedsuo
    derrrrp
    @tsloughter I actually wrote the context example like this in an earlier draft, wasn’t sure if it helped or hindered:
    func HandleUpstreamRequest(context, request, project) -> (context) {
      extract = Propagation::GetHTTPExtractor()
    
      contextWithExtract = extract(context, request.Headers)
    
      contextWithStartSpan = Tracer::StartSpan(contextWithExtract, [span options])
    
      version = Baggage::GetBaggage( contextWithStartSpan, "client-version")
    
      switch( version ){
        case "v1.0":
          contextWithFetch, data = FetchDataFromServiceB(contextWithStartSpan)
        case "v2.0":
          contextWithFetch, data = FetchDataFromServiceC(contextWithStartSpan)
      }
    
      contextWithResponse = request.Response(contextWithFetch, data)
    
      // End the current span
      contextWithEndSpan = Tracer::EndSpan(contextWithResponse)
    
      return contextWithEndSpan
    }
    Tristan Sloughter
    @tsloughter
    that might help be clearer for the explicit case
    Ted Young
    @tedsuo
    Yeah, I think it might help your edge case question but maybe confuse people in other cases? ¯_(ツ)_/¯
    Tristan Sloughter
    @tsloughter
    hah true
    Ted Young
    @tedsuo
    (aside: I’m intruiged by gitter’s coloring of my pseudocode. Not half bad! I wonder what language it thinks it is looking at.)
    Tristan Sloughter
    @tsloughter
    but ok, if this is the case then I went nuts last night for no reason, haha. I can implement this fine in Erlang and will have an additional function for like with_ctx that takes a function in which the context changes are solely kept within it
    it must think it is Go because of func
    Ted Young
    @tedsuo
    Yeah, I am adding an example pointing this out: even when context is automated, it is possible to still get an explicit handle to the current context, and how additional with_ctx functions may be necessary for handling some edge cases.
    AKA, it’s fine to write functions like MergeBaggage(context, context) -> (context) even when automated propagation is available.
    mergedContext  = MergeBaggage( Context::GetCurrent(), otherContext)
    Context::SetCurrent(mergedContext)
    Tristan Sloughter
    @tsloughter
    right, ok
    Ted Young
    @tedsuo
    holy crap, progress.
    Tristan Sloughter
    @tsloughter
    haha
    last question from me (for now :), is my removing the line about the Tracer being how span context is propagated correct?
    Ted Young
    @tedsuo
    Remove from where? Can you point?
    Tristan Sloughter
    @tsloughter
    actually, now that I've finished implementing and tests pass, I guess the span context is still set in the tracer. in my case the tracer module is basically like the baggage and correlations module but for span context
    Ted Young
    @tedsuo
    Yeah that sounds right. The tracer basically manipulates the state inside the context, while creating side effects such as sending the data somewhere, executing methods on SpanProcessor plugins, etc.
    Tristan Sloughter
    @tsloughter
    right, ok
    Ted Young
    @tedsuo
    But now the tracer/spancontext/etc no longer not need to be involved with the inject or extract APIs, like they were in the past. Instead, everything interacts with a shared context object.
    Tristan Sloughter
    @tsloughter
    is context also supposed to have a "noop" implementation in the API?
    Ted Young
    @tedsuo
    Ideally, context is also an interface
    Tristan Sloughter
    @tsloughter
    ok. that is what I did but then realized it likely didn't work that way in Go, but maybe it does?
    Ted Young
    @tedsuo
    but there should be a default implementation which is always on, imho
    Tristan Sloughter
    @tsloughter
    hm
    yea, I saw in the Ruby impl that it defaults to a noop
    so wasn't sure what goes in the API vs SDK for context
    Ted Young
    @tedsuo
    I mean, its an open question, but I’m not sure that a noop makes sense. It does for observability, but not the context layer.
    Tristan Sloughter
    @tsloughter
    ok. yea, agreed
    Tristan Sloughter
    @tsloughter
    just gotta do the propagators open-telemetry/opentelemetry-erlang-api#4
    Ted Young
    @tedsuo
    Tristan Sloughter
    @tsloughter
    cool. main difference that I think could apply to other impls is baggage/correlations/trace all use an impl of context, there is no manager of those 3
    Carlos Alberto Cortez
    @carlosalberto

    Btw:

    1) It might be good, whenever possible, to use whatever the language uses for context propagation (Context in Go), and only offer an abstraction when there's no such thing out of the box. For Python, for example, it could be contextvars (although they are not supported in all versions yet :( )
    2) I'm still not sure about the default for Context. Ideally, we could provide a good default while allowing users to provide their entire different implementation, but that might not be so easy to do in practice.

    Ted Young
    @tedsuo
    @carlosalberto that is a good point, I’m adding a clarification. If there is a default in the language, it should be used. If there is not, we need to bring a default implementation. But, we should always provide an interface which can be configured, if the langauge default is not so universal as to completely settle the matter.
    Ted Young
    @tedsuo

    I’ve been thinking more about default vs Noop. I feel that the requirements for context, propagation, and observability are actually very different. Observability is write-only, and any default implementaion would bring overhead with it. So a noop implementtion is both effect-free and useful.

    The Context object is different, because it is read-write. A “noop” would actually change behavior in observable ways: If you write a key, but then read back a nil or blank, that violates the read-write contract. So a Noop is not correct here. Users would have to wrap every call in a guard.

    Propagation is also different. There is no correct or single default to choose - each propagator is tied to an aspect. There is also the issue of suprise - if I have not enabled an aspect, it would be suprising to discover that upstream traffic could leverage that aspect to manipliuate the data I am sending downstream. We don’t have a very good handle on what the risks are, so I recommend we go with the least suprising behavior: only propagate things for aspects that have been enabled. Observability is a Noop by default, so it’s default proagators should also be a noop, even though Trace-Context headers are getting standardized.

    BTW, there's a thread going about that last point on propagators. My two cents here: https://github.com/open-telemetry/opentelemetry-specification/issues/208#issuecomment-540103625
    TL;DR: Context should be the most common system implementation by default, observability and propagation should be noop by default.
    Yusuke Tsutsumi
    @toumorokoshi

    @tedsuo reviewed! I really like how it's written as well, very articulate and I think the points are well illustrated.

    My one concern is the chained propagator stuff. II hope my concerns are well articulated in the comments, but happy to hash that one wherever you feel is appropriate.

    Tristan Sloughter
    @tsloughter
    Oh yeah, I had some too. But figured it might just be language dependent whter to re require explicit chaining or just take a list.
    Ted Young
    @tedsuo
    @toumorokoshi thank you, I appreciate it! I will try to get to you comments soon; I’ve been sucked into the vortext of the Observability Summit. :)
    @tsloughter I added a few more examples at the bottom, to clarify the edge cases you raised around automated context: https://github.com/tedsuo/rfcs/blob/context-prop-2/text/0066-separate-context-propagation.md#Examples
    rghetia
    @rghetia
    we need volunteer to take the ownership of go-prototype for context-propagation (open-telemetry/opentelemetry-go#297). The tricky part is the compatibility with OpenTraceBridge and OpenTraceBaggage. It would help if someone familiar with both can pick it up. Thx.
    Tristan Sloughter
    @tsloughter
    is there no defined name and format for encoding the correlation and baggage data into http headers? is that to come in the actual spec after the otep is accepted?
    Ted Young
    @tedsuo
    Hi y’all. I’m functioning again, post-observability-summit. Thank you for all the feedback submitted, I will be spending today processing and responding.
    Bogdan Drutu
    @bogdandrutu
    Ted Young
    @tedsuo
    @bogdandrutu what should we be looking at in that PR? Is it related to context propagation?