perf(logging): cap telemetry requests at 1 sec MONGOSH-3454 - #2803
Merged
Conversation
nbbeeken
changed the title
nbbeeken
marked this pull request as ready for review
July 28, 2026 19:27
|
Assigned |
nbbeeken
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces telemetry send/flush timeouts to cap logging-related network overhead, and adds a new startup/performance smoke test that exercises db.hello() when run from a script file.
Changes:
- Reduce telemetry request timeout from 5s to 1s and flush timeout from 2s to 1s.
- Add a performance smoke test that runs
db.hello()via--file=$INPUT_AS_FILEinplain-vmJS context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/logging/src/telemetry-client.ts | Lowers telemetry request + flush timeouts to 1s. |
| packages/cli-repl/src/smoke-tests.ts | Adds a new perf smoke test covering db.hello() executed from a script file. |
Comments suppressed due to low confidence (1)
packages/logging/src/telemetry-client.ts:11
- The PR description mentions adding a performance test, but it doesn’t mention the behavior change of reducing telemetry request/flush timeouts to 1s. Please update the PR description (or add a brief note in code/release notes) so reviewers and downstream consumers understand the motivation and expected impact (e.g., increased telemetry drop rate vs. improved shutdown latency).
export const REQUEST_TIMEOUT_MS = 1_000;
const FLUSH_TIMEOUT_MS = 1_000;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nbbeeken
requested review from
alenakhineika
and removed request for
Sgrinfy and
esvm
July 28, 2026 20:08
alenakhineika
approved these changes
Jul 29, 2026
esvm
approved these changes
Jul 29, 2026
| private readonly flushTimeoutMs: number; | ||
| private readonly controller = new AbortController(); | ||
| private pending = 0; | ||
| private pending = new Set<Promise<unknown> | undefined>(); |
Collaborator
There was a problem hiding this comment.
Is the | undefined really necessary here?
Collaborator
Author
There was a problem hiding this comment.
oops, from a previous iteration where the promise could be falsey
alenakhineika
approved these changes
Jul 29, 2026
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.
Adds a performance test that runs a db.hello() from a script file
MONGOSH-3454