Share BaggageContext across multiple NIO handlers about gsoc-swift-tracing HOT 9 OPEN

I think this sounds reasonable and we will need something like this accessible from SwiftNIO. As described above, I think we want the following properties:

• one baggage context per Channel, accessible from all ChannelHandlers through ChannelHandlerContext
• not thread safe
• because not thread safe, we use ChannelHandlerContext as the "access point" (ie. context.baggage or so) but we can't store it inside the ChannelHandlerContext because we want one per Channel
• again, because it's not thread-safe, we can't expose it through Channels API (because it's thread safe). Things like this usually go on the ChannelCore which is a public type but is not publicly accessible.

So I could see adding something like baggage { get set } to ChannelCore. Because at least right now we can't break the API, we will need a default implementation for Channel types that don't implement the then-new baggage property.

Actual users would get it through something like

// the code for the extension lives inside SwiftNIO so it can access the "private" `_core` property on channel.
extension ChannelHandlerContext {
   var baggage: ... {
       get {
           return self.channel._core.baggage
       }
       set {
            self.channel._core.baggage = newValue
      }
}

Of course that would mean that SwiftNIO will need to depend on the actual BaggageContext type somehow. Of course this is something that needs to be discussed with the SwiftNIO community but I wouldn't expect major pushback there. Having the BaggageContext as minimal as possible would of course help.

My suggestion would be to implement the whole thing in a SwiftNIO draft pull request and to propose it that way. I'm only suggesting to do this as a PR to SwiftNIO because after the BaggageContext type is created, I don't think there's much work in SwiftNIO itself at all actually. If you disagree and think that this would be lots of work to create the PR and you'd like to discuss it before writing the code, that's totally cool too. In that case I'd recommend a forums post or a github issue.

Let me also CC @Lukasa, @glbrntt, @Davidde94, @normanmaurer

How does that sound to everybody?

from gsoc-swift-tracing.

This is important enough that I'd also be open to make this a branch on github.com/apple/swift-nio rather than only your personal forks?

Nah, private is good enough I think. We'll iterate a bit this way and see how things fit.

Good observation about the read only if we ever exposed it on Channel :)
Having that said I don't think we need it there as I mentioned above, just wanted to make clear we don't put it there (until someone has actual strong reason to, and then read only anyway).

Agreed on not adding anything except the minimum as well.

from gsoc-swift-tracing.

Some extra references:

This would mirror the Netty feature of "attributes", which are:

Storing stateful information

AttributeMap.attr(AttributeKey) allow you to store and access stateful information that is related with a handler and its context. Please refer to ChannelHandler to learn various recommended ways to manage stateful information.

https://netty.io/4.0/api/io/netty/channel/ChannelHandlerContext.html#storing-stateful-information

In Netty a channel handler context extends AttributeMap but in our case I think it's more likely that the baggage would be a property inside of the ChannelHandlerContext I guess. It's also another case of why we're trying to not use the word context -- to avoid (nio)context.(baggage)context.

It would seem this would want to be added to baggage { get set } to ChannelCore https://github.com/apple/swift-nio/blob/a91c87f1c297c4326c4bbdec9c0dcb12f32c6a2c/Sources/NIO/Channel.swift#L22 as @weissi mentioned to me some time earlier.

Do we want to also expose it form Channel but "make it automatically hop to the EL if needed"? I'm not sure we need it on channel tbh and on the context could be fine.

Thanks in advance for any hints @weissi and @slashmo hopefully can have a stab at this.
It would not be merge-ready until the context library is "hardened and ready" so we'd for now be developing things against @slashmo's branch with the context on there. Once we're all happy with things we can figure out how/where the libs live and then it could become "real" in NIO I suppose :)

from gsoc-swift-tracing.

Agreed on all of the above :-) Indeed should not be too much work; and a Draft PR likely good. I'm guessing though it would not be merged for months (at-least end of GSoC + "maturing" + accepting by SSWG of the baggage type etc) -- sanity checking you don't mind such lingering around PR. WIP PR is good enough for us to proceed prototyping -- we can depend on the branch after all :)

re:

again, because it's not thread-safe, we can't expose it through Channels API (because it's thread safe). Things like this usually go on the ChannelCore which is a public type but is not publicly accessible.

Just sanity checking on this one (as I don't think we need it AFAICS) so should be fine to skip. @Lukasa mentioned when we chatted that it may be useful to also offer on channel and "make it safe" using the if eventLoop.inEventLoop { ... } else { eventLoop.submit { ... dance.

from gsoc-swift-tracing.

Agreed on all of the above :-) Indeed should not be too much work; and a Draft PR likely good.

Yeah, just gives us something concrete to discuss.

I'm guessing though it would not be merged for months (at-least end of GSoC + "maturing" + accepting by SSWG of the baggage type etc) -- sanity checking you don't mind such lingering around PR. WIP PR is good enough for us to proceed prototyping -- we can depend on the branch after all :)

I don't mind it at all, can't speak for the others. But our oldest open PR is from July 2019 in the swift-nio and I bet there's older ones somewhere in swift-nio-* I think it's fine.

This is important enough that I'd also be open to make this a branch on github.com/apple/swift-nio rather than only your personal forks? That'd need discussing as we usually don't do random branches apart from the main branch and nio-X.Y for patches to old minor releases.

re:

again, because it's not thread-safe, we can't expose it through Channels API (because it's thread safe). Things like this usually go on the ChannelCore which is a public type but is not publicly accessible.

Just sanity checking on this one (as I don't think we need it AFAICS) so should be fine to skip. @Lukasa mentioned when we chatted that it may be useful to also offer on channel and "make it safe" using the if eventLoop.inEventLoop { ... } else { eventLoop.submit { ... dance.

We could but then we'd need to pick one of those options:

• make it async and return EventLoopFuture<BaggageContext>
• make it sync and block until we're on the EL which means it's only safe to access either on the correct EL or on a random thread that's not an EL

Both of those aren't great I think because ChannelHandlerContext wants to just synchronously get it and to enable that we'd need to kinda add something sync (but only allowed on EL) to ChannelCore anyway. If it's really useful to get the BaggageContext from potentially other ELs as a future, then we can at a later point always add

extension Channel {
    var baggage: ELF<BaggageContext> {
        if self.inEventLoop {
            return self.channel.eventLoop.makeSucceededFuture(self._core.baggage)
       } else {
           return self.eventLoop.submit { self._core.baggage }
       }
    }
}

If useful, I'd see that as a natural extension that can be done at any time. And also I'd make it read only: Don't think someone outside of the pipeline should just write to it :)

from gsoc-swift-tracing.

Is there a schedule for that? :-D
It should be straight forward to make an experimental version

I made PoC tracing for AWS Lambda handler, it would be great pass context/baggage in channel,
HelloWorldAPIPrefHandler should be child of HandleEvent and its a bit problematic to have "current segment/span" in NIO world.

from gsoc-swift-tracing.

What is "that" specifically -- you mean baggage being merged into an actual NIO release?
Because the PoC you can have a look at right now: apple/swift-nio#1574

An actual merge can only happen after it goes through SSWG things, NIO team are happy with it and it is stable enough to not cause versioning trouble for NIO, a rough guesstimate would be a few months I guess; One month to finish the GSoC phase, some wiggle time for SSWG and NIO feedback rounds and then it's likely good.

What would actually help to make it happen (and quicker, and safer) is real use cases, such as the one you posted, doing the following:

• depend on apple/swift-nio#1574
• show in your example how you'd use it
• then we can prove to NIO team and everyone else that yes, this addition is super helpful and it is important to get in soon rather than keep waiting.

from gsoc-swift-tracing.

Because the PoC you can have a look at right now: apple/swift-nio#1574

awesome, this is exactly what I asked about
if its ready to use I will give it a try

from gsoc-swift-tracing.

Let me try to use it from slashmo:feature/baggage-context first and will give feedback on that.

@slashmo

Would be great if was synced with NIO master, currently its:

This branch is 2 commits ahead, 25 commits behind apple:master.

@ktoso

If it works well and given ti will take give or take few months before it will be merged, maybe it could land in experimental/tracing branch in NIO repo for now.

from gsoc-swift-tracing.