make handler spans more accurate, re-add connection span

[jackyzha0]

Why

• a lot of proc handler specific log messages didnt get picked up by the right tracer
• move proc stream <> handler proxy messages into the handler span
• move the invoked message into the client handler span
• re-add connection telemetry
• span linking between procs/handlers and their originating sessions
• we use https://www.npmjs.com/package/@opentelemetry/context-async-hooks so context is tracked safely in an async context so we should leverage that

What changed

Versioning

• Breaking protocol change
• Breaking ts/js API change

[jackyzha0]

this didn't actually test anything useful and we dont have an easy way to create a mock session to replicate this so i ended up just yoinking it, the integration test below tests more of the flow

[blast-hardcheese]

Looks sensible, one comment

[blast-hardcheese]

I would not expect an innocuous logging function to modify the metadata here. How are these metadata created? Can we just be more strict instead of trying to heal post-hoc? Removing optionality increases intelligibility

[jackyzha0]

this metadata are manually injected at logging callsites, theres a lot and we kept finding places where we didnt inject the right telemetry :(

i thought it would be easier to 'do the right thing' and follow the open telemetry suggestion of just always logging the active telemetry

this doesnt log anything if its not within an active span

[jackyzha0]

it also means that any logging inside a user-written handler also gets the right span associations

[blast-hardcheese]

I still wouldn't expect it, is the concern that they're too expensive to preallocate or what?

[jackyzha0]

no more so its hard to remember to inject the telemetry info at every place we care about it so i thought it made sense to do it implicitly because the implicit approach will always have the right telemetry

is the worry here that we are modifying the param or that its just doing patching? we do some cleaning here already below this (stripping the payload)

[blast-hardcheese]

Yeah, the underlying concern is the implicit complexity. In a logging method, we take an optional parameter with an optional member, then we might write that field before returning back.

A logging function definitely doesn't seem to be the right place to be doing any of that work.

Ultimately it's not the end of the world if we go this way, but I'd much rather solve this by making it easier to create and manage the MessageMetadata instead of offering important functionality for free invisibly just by calling log. If that's easy, then the span fields could even be marked required and defensive complexity can be delegated to where it belongs.

[lhchavez]

of note, this is exactly how most third-party logging libraries operate: they add a global hook where they perform this same operation of massaging the metadata, only to avoid callers having to reason about adding metadata explicitly so that all logs get the tracing metadata.

[lhchavez]

is there still a way to link them (maybe starting an async span or using links)? or is it not desirable to begin with? (if it's the latter, maybe it's worth expanding the code comment explaining the rationale / consequences)

[jackyzha0]

yes! i can add a comment but we add the link already in span itself, see createProcTelemetryInfo

[lhchavez]

of note, this is exactly how most third-party logging libraries operate: they add a global hook where they perform this same operation of massaging the metadata, only to avoid callers having to reason about adding metadata explicitly so that all logs get the tracing metadata.