CLDSRV-981: Log Scuba health checks through the server logger - #6261
Closed
anurag4DSB wants to merge 2 commits into
Closed
CLDSRV-981: Log Scuba health checks through the server logger#6261anurag4DSB wants to merge 2 commits into
anurag4DSB wants to merge 2 commits into
Conversation
Contributor
Hello anurag4dsb,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #6261 +/- ##
===================================================
+ Coverage 85.17% 85.19% +0.02%
===================================================
Files 206 206
Lines 13424 13423 -1
===================================================
+ Hits 11434 11436 +2
+ Misses 1990 1987 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
ScubaClientImpl.setup(log) stored the werelogs RequestLogger that server.js creates at boot for initiateStartup, and the periodic health check timer then logged through it on every enable/disable transition. RequestLogger buffers every entry it is handed and only drains on an error-level write, so the buffer grew for the process lifetime and was never reclaimed. Measured: 199 entries retained after 200 health check flaps. The health check runs on a timer, not inside a request, so it takes the module-level server logger, which writes through instead of buffering. setup() no longer needs a logger argument.
Adds regression tests: the client retains no logger it is handed, health check transitions do not write through one, and the enabling-quotas line fires on the disabled-to-enabled transition. The four existing _healthCheck cases had been failing on development/9.3 because sinon reports healthCheck as non-existent - it sits deep in the scubaclient prototype chain - so they never ran. They assign the method directly instead. They cover the function this change touches, so they are worth having alive: tests/unit goes from 5114 passing / 15 failing to 5120 / 11.
anurag4DSB
force-pushed
the
bugfix/CLDSRV-981-scuba-pinned-startup-logger
branch
from
August 20, 2026 14:21
28f4fba to
4b4ebb8
Compare
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
2 participants
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.
Intent: why does this change exist?
The quota client held the werelogs
RequestLoggerthatserver.jsbuilds at boot, and its health-check timer logged through it forever. Same bug class as CLDSRV-979, found in the same RD-2240 investigation, much smaller blast radius.System impact: what's affected, including downstream?
lib/utilization/scuba/wrapper.jsand its one call site inlib/server.js.ScubaClientImpl.setup()no longer takes a logger argument —lib/server.js:403is the only production caller. Health-check log lines now carry the server logger's fields instead of the startup request's, so they no longer have areq_id; anything grepping these two messages byreq_idwould be affected, though thatreq_idwas misleading anyway since it belonged to an unrelated boot-time request.Preserved behavior: what explicitly stays the same?
The health check itself — cadence, staleness threshold, and when quotas get enabled or disabled. Both messages keep their text and their levels.
Intended change: what's different after this PR?
Background health-check logging goes through the module-level server logger, which writes through and drops sub-level entries rather than buffering them. Nothing that outlives a request holds a
RequestLogger.Verification: how do we know this worked, or how would we know if it didn't?
Reproduced first: 200 simulated health-check flaps against the pinned startup logger left 199 buffered entries, every one carrying the same
req_id— which is the signature of this bug generally, and a cheap thing to grep for elsewhere.Two regression tests cover it. While in the file I also revived the four
_healthCheckcases that had been silently failing ondevelopment/9.3: sinon reportshealthCheckas non-existent because it sits deep in the scubaclient prototype chain, so they assign the method directly instead. Those four cover the function this PR changes, which is why they are in scope rather than left for later.tests/unitgoes from 5114 passing / 15 failing to 5120 / 11; the remaining 11 are the pre-existingbucketPutandbucketUpdateQuotaquota-metric-seeding failures, untouched by this. Lint clean, 0 errors.