[Proposal] SDT-0001: task-local instrument

Server Development
1

Hi all,

The revised proposal SDT-0001: task-local instrument for swift-distributed-tracing is now up and In Review.

The review period will run until July 6th — please feel free to post your feedback as a reply to this thread.

Thanks!

2

So... this needs more debate for sure :sweat_smile:

I'd invite folks from @server-workgroup also to chime in as this is a rather important proposal.

Please hold on it a bit Vlad, we talked about it but please give me a day or two to make double sure the final shape is what we can be confident about :folded_hands: Sorry for the delay on my end, some "life stuff" was happening recently on my end.

Also ping @slashmo, something that would be good to have a look at for you.

1 Like
3

For the context — there is an active discussion in the linked PR.

1 Like
4

Heh, I see a discussion happening over there rather than in the review thread... took me by surprise a bit, I missed that -- thanks for pointing it out.

Let me check what's up there and what we did with the multiplex one so far, thank you :slight_smile: Just making sure the final bits are good -- we talked over this and overall it was looking good already!

5

This has been open well past the review deadline. After some offline conversations with @ktoso and @franzbusch, the proposal discussion deadline is extended until the 20th of July.

6

I finally got around to reviewing this proposal. Overall, I agree that we should solve this for both testing purposes but also for purposes where libraries or application authors want to use an instrument for a specific scope e.g. set an in-memory instrument for a scope, wait how the scope returns, then on error flush all the recorded spans to the "parent" instrument. The latter would allow the collection of very detailed sub-spans only on error paths. Doing this on an application level with a globally bootstrapped backend is almost impossible.

I think the proposed public API looks good but I am concerned about this documentation comment and the underlying semantics:

/// > Important: This is an application-level facility — call it from code that owns the instrumentation setup,
/// > never from a library. It can install its instrument only when the ``InstrumentationSystem`` is otherwise
/// > un-bootstrapped. Calling it after a plain ``Instrument`` has been bootstrapped crashes. Libraries emit
/// > through ``InstrumentationSystem/instrument`` and `withSpan` / `startSpan`, which already observe whatever
/// > is active.

I think this would mean that the above use case won't be possible to implement. Making the task-local instrument an application-only tool is in my opinion the wrong choice. I understand that our concern is around the performance of an additional task-local lookup. I continue to think that this is the wrong trade-off here. We should find ways similar to swift-log to disable trace recording at compile time to make them truly no-ops. In my opinion, it is more important that the semantic behavior between logging and tracing are up-held i.e. withLogger and withInstrument behave semantically similar.

To reduce the amount of task-local look-ups a unified "telemetry" library for Swift could help us get it down to a single one but that is in my opinion a future direction that we can take once we exhausted all other avenues of performance tuning and the task-local look-ups turn out to be performance prohibitive.

1 Like
7

If we're particularly concerned about adding an extra task local lookup, I wonder if storing the Tracer in the ServiceContext would be viable. In a way, it's not dissimilar to logging, where the value type that's passed around (Logger) has a ref to the backend (LogHandler). Similarly here, I consider the ServiceContext the value type we pass around, so it storing the Tracer ref wouldn't be without precedent, and I think it'd address the concerns that pushed this proposal to avoid the extra lookup.

8

Thanks all, and @franzbusch and @czechboy0 especially for the pushback.

Closing the review today rather than extending again. In its current form SDT-0001 isn't accepted.

The revision will keep the API, but drop the special instrument and moves to an always-on task-local. I'll rework and re-open review.

1 Like