Skip to content

Conversation

@keelerm84
Copy link
Member

@keelerm84 keelerm84 commented Nov 7, 2025

Note

Plumbs diagnostic instrumentation through FDv1/FDv2 and the FDv2 streaming data source, recording stream-init metrics and wiring the accumulator into synchronizers, with protocol/typing and test updates.

  • Diagnostics:
    • Introduce DiagnosticAccumulator and DiagnosticSource protocols in ldclient.impl.datasystem (runtime-checkable) and add setter APIs.
    • Add optional diagnostic accumulator to FDv1/FDv2; provide set_diagnostic_accumulator and propagate to synchronizers that implement DiagnosticSource.
  • Streaming (FDv2):
    • Make StreamingDataSource implement DiagnosticSource; track connection attempt start time and record record_stream_init on success/failure (using next_retry_delay).
    • Handle Start fallback header X-LD-FD-Fallback by recording failure and yielding revert_to_fdv1=True.
    • Enhance error handling to record diagnostics and manage retry timing for JSON errors, HTTP errors, and unexpected errors; clear timing on permanent failures.
  • Protocol/typing:
    • Define Synchronizer.sync(ss) in protocolv2 with proper Generator[Update,...] typing and TYPE_CHECKING imports.
  • Tests:
    • Update test SSE client to expose next_retry_delay; adjust streaming synchronizer tests accordingly.

Written by Cursor Bugbot for commit 383a396. This will update automatically on new commits. Configure here.

@keelerm84 keelerm84 requested a review from a team as a code owner November 7, 2025 20:33
log.error("Unexpected error on stream connection: %s, will retry", error)
self._record_stream_init(True)
self._connection_attempt_start_time = time() + \
self._sse.next_retry_delay # type: ignore
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Ensure Accurate Connection Attempt Timing

After recording a failed stream initialization, _connection_attempt_start_time is set directly to a future time without first setting it to None. This differs from the v1 implementation and could cause incorrect elapsed time calculations if _record_stream_init is called again before the next connection attempt starts. The v1 pattern sets it to None first to prevent using the future timestamp in subsequent diagnostic recordings.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only seems that way. If you look carefully, you will see that the v1 streaming implementation sets this to None for the JSONDecodeError branch, but eventually falls through to the line that does

        self._connection_attempt_start_time = time.time() + self._sse.next_retry_delay

I just cut out the middle assignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants