Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): switch ping-pong latency aggregation to per-socket #23856

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

znewton
Copy link
Contributor

@znewton znewton commented Feb 13, 2025

Description

Switches the R11s Socket latency telemetry from global aggregation (average over time) to per-socket aggregation (average over threshold ping-pong events). This allows for better telemetry granularity (percentiles, tenant/document narrowing, etc.) at the cost of more generated telemetry. The amount of telemetry can be tuned with the pingPongLatencyTrackingAggregationThreshold config, where 1 ping-pong event should happen every ~25 seconds.

Breaking Changes

An internal param that was never used was removed, and a config that was never set was removed.

@Copilot Copilot bot review requested due to automatic review settings February 13, 2025 23:06
@github-actions github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Feb 13, 2025

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • server/routerlicious/packages/routerlicious/config/config.json: Language not supported
Comments suppressed due to low confidence (2)

server/routerlicious/packages/services-shared/src/socketIoServer.ts:327

  • Ensure that the new behavior introduced by pingPongLatencyTrackingAggregationThreshold is covered by tests.
private initPingPongLatencyTracking(

server/routerlicious/packages/services-telemetry/src/lumberEventNames.ts:65

  • [nitpick] The telemetry event name SocketConnectionLatency should be consistent with existing naming conventions. Consider renaming it to SocketIoConnectionLatency.
SocketConnectionLatency = "SocketConnectionLatency"
@znewton znewton requested a review from a team as a code owner February 13, 2025 23:11
Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

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

Approved for docs.

@znewton znewton merged commit 6ab8453 into microsoft:main Feb 14, 2025
34 checks passed
@znewton znewton deleted the server/ping-latency-sampling branch February 14, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch changeset-present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants