feat(server): private match telemetry stream#4687
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds opt-in match telemetry with typed events, validated configuration, bounded signed batching, lifecycle and intent instrumentation, worker integration, and automated tests. ChangesMatch telemetry
Repository hygiene
Estimated code review effort: 5 (Critical) | ~90 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
614ea71 to
418aae3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/telemetry/MatchTelemetry.ts`:
- Around line 94-117: Replace the shared mutable ZERO_COUNTERS usage in
noopMatchTelemetryEmitter with a true no-op emitter: emit must return "dropped"
without mutating module-level state, and counters must return a fresh empty
MatchTelemetryCounters object for each call. Keep stop unchanged and ensure
disabled emitters do not share totals across GameServer or GameManager
instances.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23378fcc-19da-4944-b1fb-473eccc035cd
📒 Files selected for processing (11)
.gitignoreexample.envsrc/server/GameManager.tssrc/server/GameServer.tssrc/server/Worker.tssrc/server/telemetry/BufferedMatchTelemetryEmitter.tssrc/server/telemetry/MatchTelemetry.tssrc/server/telemetry/MatchTelemetryConfig.tstests/server/MatchTelemetryIntegration.test.tstests/server/telemetry/BufferedMatchTelemetryEmitter.test.tstests/server/telemetry/MatchTelemetryConfig.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
418aae3 to
5127bb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/server/telemetry/MatchTelemetryConfig.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic number duplicated with the websocket payload cap.
MAX_WEBSOCKET_PAYLOAD_BYTES(1MB) mirrors themaxPayload: 1024 * 1024set on theWebSocketServerinWorker.ts. Since these two are only related in intent (not in code), they can silently drift if one is changed without the other.♻️ Suggested fix: share the constant
-const MAX_WEBSOCKET_PAYLOAD_BYTES = 1024 * 1024; +export const MAX_WEBSOCKET_PAYLOAD_BYTES = 1024 * 1024;Then import and reuse it in
Worker.ts'sWebSocketServerconstruction instead of the separate literal.Also applies to: 31-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/telemetry/MatchTelemetryConfig.ts` at line 1, Export MAX_WEBSOCKET_PAYLOAD_BYTES from MatchTelemetryConfig and update the WebSocketServer construction in Worker.ts to import and reuse it for maxPayload, removing the duplicated 1024 * 1024 literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/GameServer.ts`:
- Around line 606-617: Update the default gameplay-intent path in GameServer’s
intent handling so paused intents receive a distinct telemetry reasonCode such
as "paused" instead of being indistinguishable from queued accepted intents.
Preserve the existing admin-bot rejection and queue behavior for unpaused
intents, and ensure the reason is attached to the outcome returned by finish.
In `@src/server/telemetry/MatchTelemetryConfig.ts`:
- Around line 73-74: Update the TELEMETRY_SIGNING_SECRET validation in
MatchTelemetryConfig to reject secrets shorter than the required minimum
strength, such as 32 characters, and return disabled with a clear validation
message. Preserve the existing enabled configuration path for secrets meeting
the minimum length.
---
Nitpick comments:
In `@src/server/telemetry/MatchTelemetryConfig.ts`:
- Line 1: Export MAX_WEBSOCKET_PAYLOAD_BYTES from MatchTelemetryConfig and
update the WebSocketServer construction in Worker.ts to import and reuse it for
maxPayload, removing the duplicated 1024 * 1024 literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c659b925-85d1-484e-8f57-3879c299de02
📒 Files selected for processing (12)
.gitignoreexample.envsrc/server/GameManager.tssrc/server/GameServer.tssrc/server/Worker.tssrc/server/telemetry/BufferedMatchTelemetryEmitter.tssrc/server/telemetry/MatchTelemetry.tssrc/server/telemetry/MatchTelemetryConfig.tstests/server/MatchTelemetryIntegration.test.tstests/server/telemetry/BufferedMatchTelemetryEmitter.test.tstests/server/telemetry/MatchTelemetry.test.tstests/server/telemetry/MatchTelemetryConfig.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- .gitignore
- src/server/Worker.ts
- example.env
- src/server/GameManager.ts
- tests/server/telemetry/MatchTelemetryConfig.test.ts
- src/server/telemetry/BufferedMatchTelemetryEmitter.ts
- tests/server/telemetry/BufferedMatchTelemetryEmitter.test.ts
- tests/server/MatchTelemetryIntegration.test.ts
5127bb1 to
2f86d52
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/Worker.ts`:
- Around line 62-67: Update the telemetry configuration in the Worker
initialization to pass the existing normalized workerId local value instead of
calling ServerEnv.workerId() again. Keep the routing identity and telemetry
identity consistent when the environment value is absent.
- Line 69: Update startWorker and its shutdown/fatal-exit flows to explicitly
close the worker server and await telemetry completion rather than relying only
on the server "close" handler. Extend the telemetry emitter lifecycle with an
asynchronous flush/stop operation that drains already-enqueued events before
exit, and invoke it during shutdown while preserving prevention of future
batching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d92b9596-528f-4284-a238-b0ff3c12a8bb
📒 Files selected for processing (12)
.gitignoreexample.envsrc/server/GameManager.tssrc/server/GameServer.tssrc/server/Worker.tssrc/server/telemetry/BufferedMatchTelemetryEmitter.tssrc/server/telemetry/MatchTelemetry.tssrc/server/telemetry/MatchTelemetryConfig.tstests/server/MatchTelemetryIntegration.test.tstests/server/telemetry/BufferedMatchTelemetryEmitter.test.tstests/server/telemetry/MatchTelemetry.test.tstests/server/telemetry/MatchTelemetryConfig.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/server/telemetry/MatchTelemetry.test.ts
- .gitignore
- src/server/GameManager.ts
- example.env
- src/server/telemetry/MatchTelemetryConfig.ts
- tests/server/telemetry/MatchTelemetryConfig.test.ts
- src/server/telemetry/MatchTelemetry.ts
- tests/server/MatchTelemetryIntegration.test.ts
- src/server/telemetry/BufferedMatchTelemetryEmitter.ts
- tests/server/telemetry/BufferedMatchTelemetryEmitter.test.ts
- src/server/GameServer.ts
Add a generic, private match-telemetry stream for operational observability. It records match lifecycle events and server-ingress intent decisions and ships them asynchronously to a single authenticated ingest endpoint. - Best-effort and bounded: a signed, size-bounded buffered emitter that never blocks gameplay. - Opt-in via environment variables (see example.env); disabled by default, with no change to server behavior when off. - The deterministic public replay (GameRecord) is unchanged. Includes unit and integration tests for the emitter, config, and server wiring.
2f86d52 to
d1bd49f
Compare
|
@coderabbitai resolve |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedComments resolved and changes approved. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/GameServer.ts`:
- Around line 235-241: Update identityFor to hash client.persistentID before
assigning it to the telemetry identity, using the existing shared hashing helper
for opaque player keys. Ensure all identifiers emitted through telemetry
identity fields are hashed and no raw persistentID reaches telemetry.emit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 502715ee-3375-4e70-8007-cc6c31f8c7c8
📒 Files selected for processing (12)
.gitignoreexample.envsrc/server/GameManager.tssrc/server/GameServer.tssrc/server/Worker.tssrc/server/telemetry/BufferedMatchTelemetryEmitter.tssrc/server/telemetry/MatchTelemetry.tssrc/server/telemetry/MatchTelemetryConfig.tstests/server/MatchTelemetryIntegration.test.tstests/server/telemetry/BufferedMatchTelemetryEmitter.test.tstests/server/telemetry/MatchTelemetry.test.tstests/server/telemetry/MatchTelemetryConfig.test.ts
|
Intentional design — not applying this one. This private telemetry stream is deliberately built to carry raw identifiers. The payload is HMAC-signed and delivered to a single authenticated HTTPS ingest endpoint, and downstream consumers depend on the raw So this is expected behavior rather than a leak. Marking as won't-fix. |
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
| return { | ||
| clientId: client.clientID, | ||
| publicId: client.publicId, | ||
| persistentId: client.persistentID, |
There was a problem hiding this comment.
to be safe should we omit the persistentId?
2 participants
Add approved & assigned issue number here:
Resolves #(issue number)
Description:
Adds a generic, private match-telemetry stream to the game server for operational observability. It records match lifecycle events and server-ingress intent decisions and ships them asynchronously to a single authenticated ingest endpoint.
example.env); disabled by default, with no change to server behavior when off.GameRecord) is unchanged.Changes:
src/server/telemetry/MatchTelemetry.ts— telemetry event/payload typessrc/server/telemetry/MatchTelemetryConfig.ts— env-driven configurationsrc/server/telemetry/BufferedMatchTelemetryEmitter.ts— bounded, signed, async buffered emittersrc/server/GameServer.ts,GameManager.ts,Worker.ts— observe match lifecycle + intent outcomes and wire telemetry through the workersexample.env— new configuration entriesPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
jish