slashmo / gsoc-swift-baggage-context

BaggageContext library, extracted from slashmo/gsoc-swift-tracing
https://github.com/slashmo/gsoc-swift-tracing
Apache License 2.0
4 stars 2 forks source link

Required context parameter considerations #8

Closed kmahar closed 3 years ago

kmahar commented 4 years ago

One of the proposed API guidelines is that the context parameter be required, to avoid the context being inadvertently dropped.

There are a couple of considerations to think about here:

1) What a should a library do if they wish to integrate tracing but are not willing or able to make a breaking change yet? One solution would be to maintain both new and old versions of the method, one of which takes in the context, and deprecate the old context-free one to push users toward providing context. (Alternatively, the context parameter could be optional, but then we still have the issue of an inadvertently dropped context.)

2) Some libraries may have users/use cases where tracing isn't needed or wanted. How can library developers support users who don't already have a context around to pass in? @ktoso suggested this could be handled by providing some default "no context" value users can pass.

slashmo commented 4 years ago

Thanks for bringing this up, @kmahar!

I think we should avoid defaulting the BaggageContext argument to anything, be it nil or @ktoso's suggested .none in order to avoid dropping traces.

I also get the point about this potentially being awkward for other use cases where tracing doesn't make as much sense. Here, in my opinion, the decision should be up to a specific library whether it's worth the trade-off in order to avoid dropping traces for the server-side use of their API.

kmahar commented 4 years ago

That all makes sense to me! On a related note I am +1 to the proposed parameter ordering after reading through all of that, it feels intuitive and matches up with how I would have thought to add context parameters to the MongoDB driver APIs.

slashmo commented 3 years ago

@kmahar Not closely related to this issue, but version 0.2.0 of this package might be of interest to you. It adds BaggageContextCarrier, which allows you to pass framework contexts (holding a baggage) to user-facing APIs that need a BaggageContext.