feat: Add ServerTrace instrumentation hooks to the SSE server - #67
Open
keelerm84 wants to merge 6 commits into
Open
feat: Add ServerTrace instrumentation hooks to the SSE server#67keelerm84 wants to merge 6 commits into
keelerm84 wants to merge 6 commits into
Conversation
Add a vendor-neutral, httptrace-style ServerTrace struct of optional callbacks, exposed as the Server.Trace field. This surfaces server-side signals that were previously invisible to consumers, most importantly the silent force-drop of subscribers that fall behind the buffer. Hook surface (each takes a single info struct so fields can grow compatibly): - SubscriberAdded / SubscriberRemoved: connection lifecycle, with a typed removal reason (client_closed, max_conn_time, write_error, server_closed, unregistered, buffer_overflow) and connection duration. - SubscriberDropped: fired when a slow subscriber is force-disconnected for buffer overflow (also reported through SubscriberRemoved). - EventSent / CommentSent: per-subscriber delivery, with event type and data size but never the payload. - EventDiscarded: jitter-coalescing discards. - WriteError: encode/write failures. - ReplayStarted / ReplayFinished: replay drain, with event count and duration. An opaque per-subscription id correlates the subscriber callbacks. The Server-initiated close reason is recorded on the subscription before its channel is closed and read by the handler only after it observes the close, using the channel close as the happens-before edge. Also fill the previously-silent Logger gaps: a warn-intent message when a subscriber is dropped for buffer overflow, and debug-intent messages for subscriber add/remove and replay drain. Logging is independent of ServerTrace and encodes level intent as a message prefix. Callbacks are invoked from internal goroutines, including the single dispatch goroutine, and must return promptly. A nil ServerTrace or nil field disables the hook with zero overhead beyond a nil check. No new dependencies, no change to event delivery, and the client Stream is untouched.
The ServerTrace hooks that fire on a subscriber's connection handler goroutine now receive that subscriber's request context, and EventSent and CommentSent report the measured encode+flush duration for each write. Together these let a consumer attach correctly-timed child spans and span events to the per-connection request span without needing paired pre/post hooks. The write duration is measured only when the matching callback is set, preserving the zero-overhead nil path.
The hook surface is expected to evolve as the relay-side OTel bridge is built against it, so state plainly that callbacks, info fields, and reason values may change or be removed in any release, including a patch release.
Replayed events are encoded in bulk and flushed once per batch rather than flushed individually. That makes per-event EventSent misleading for replayed events: the "has been flushed" contract and per-event WriteDuration no longer hold, since the single batch flush happens only at the end of the batch. Report replay entirely at the batch level instead: - EventSent/CommentSent fire only for individually-flushed live writes. Replayed events no longer emit EventSent. - ReplayFinished.DrainDuration now spans through the end-of-batch flush, correctly attributing the batch flush cost to replay. - ReplayFinishedInfo gains TotalDataSize, the summed data-payload bytes of the batch, to preserve payload-size observability now that per-event DataSize is no longer reported for replayed events.
TestServerTraceEventAndCommentSent asserted a strictly positive WriteDuration for a write into an in-memory buffer. On Windows the monotonic clock advances on the system timer tick (~15.6ms), so a correct measurement of such a fast write rounds to zero and the assertion failed on both Windows jobs. traceTestWriter gains an optional per-write delay, and the test asserts a floor well below that delay: the same granularity that rounds a fast write down to zero can round a delayed write down to a single tick, so asserting the full delay would only move the flake. A zero or unmeasured duration still fails. Also assert the comment write duration, which is measured the same way.
kinyoklion
approved these changes
Aug 5, 2026
keelerm84
marked this pull request as ready for review
August 5, 2026 15:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
ServerTrace, an opt-in set of callbacks that the SSEServerinvokes at points inits lifecycle, so a consumer can observe per-connection and replay behavior. This is phase 1
of the eventsource server observability work; the OTel bridge that implements these hooks
lives in ld-relay. The library creates no spans and takes no new dependencies -- it only
invokes optional callbacks. The client
Streamis untouched.The design follows
net/http/httptrace.ClientTrace: a struct of optional function fields,exposed as a new
Server.Tracefield alongside the existingBufferSize/MaxConnTime/Loggerconfig. Payloads are
*Infostructs rather than positional arguments so fields can be addedcompatibly. A nil
ServerTrace, or a nil field within it, disables the corresponding hook.Callbacks
SubscriberAddedSubscriberRemovedSubscriberDroppedBufferSizeand is disconnectedEventSentCommentSentEventDiscardedWriteErrorReplayStarted/ReplayFinishedSubscriberRemovedInfo.Reasonis a newSubscriberRemovedReasonwith six values:client_closed,max_conn_time,write_error,server_closed,unregistered, andbuffer_overflow. Attributing these required threading an exit reason through every read-loopexit path, plus a
closeReasonrecorded on the subscription byServer.run()before it closesthe subscriber's channel; that channel close is the happens-before edge that makes the
handler-side read race-free.
Request context
Every callback that fires on a subscriber's connection handler goroutine receives that
subscriber's request context as its first argument. It is for telemetry correlation only, such
as attaching a child span to the request span, and must not be used for cancellation. The
context may already be canceled by the time a late callback such as
SubscriberRemovedruns,because the client closing the connection is itself what ends the subscription; that is
expected and does not prevent creating a span from it.
Replay is reported at batch level
Replayed events are encoded in bulk and flushed once per batch rather than individually, so
per-event
EventSentwould be misleading for them: neither the "has been flushed" contract nora per-event
WriteDurationholds when the single flush happens only at the end of the batch.Replay is therefore reported entirely through
ReplayStarted/ReplayFinished:EventSent/CommentSentfire only for individually-flushed live writes. Replayed events donot emit
EventSent.ReplayFinished.DrainDurationspans through the end-of-batch flush, so the batch flush costis attributed to replay.
ReplayFinishedInfo.TotalDataSizereports the summed data-payload bytes of the batch,preserving payload-size observability now that per-event
DataSizeis not reported forreplayed events.
Logging
The same instrumentation points feed
Server.Loggerwhen it is set, covering events that aresilent today (only encoder errors are logged):
[WARN]on a slow-subscriber drop, and[DEBUG]on subscriber add/remove and replay drain. No behavior change.Overhead and payload safety
Timings are computed only when something will consume them, so an unobserved
Serverreads noclock and makes no extra allocation on the write path. Sizes are reported as byte counts only;
no callback ever receives an event payload.
Compatibility
The whole surface is marked
EXPERIMENTALand for use by LaunchDarkly libraries only:callbacks, info fields, and reason values may be added, renamed, or removed in any release,
including a patch release, and the points at which callbacks fire may change.