Forward X-Scal-Request-Uids on Backbeat report/CRR-metrics requests#6219
Forward X-Scal-Request-Uids on Backbeat report/CRR-metrics requests#6219delthas wants to merge 3 commits into
Conversation
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
❌ 1 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Propagate the werelogs request-uid chain (log.getSerializedUids()) on the Backbeat report / CRR-metrics HTTP calls made through reportHandler's _makeRequest, via the x-scal-request-uids header. This matches the header cloudserver already reads on incoming admin requests, so req_id continuity is preserved across the Backbeat metric and state/schedule calls. Issue: CLDSRV-947
e39cb49 to
881981a
Compare
| (site, next) => | ||
| requestMethod(endpoint, site, log, (err, res) => { | ||
| if (err) { | ||
| log.debug('Error in retrieving site metrics', { | ||
| method: '_getMetricsByLocation', | ||
| error: err, | ||
| site, | ||
| requestType: requestMethod.name, | ||
| }); | ||
| return next(null, { site, stats: {} }); | ||
| } | ||
| return next(null, { site, stats: res }); | ||
| }), |
| }); | ||
| return async.parallel( | ||
| { | ||
| all: done => requestMethod(endpoint, 'all', log, done), |
| return async.parallel( | ||
| { | ||
| all: done => requestMethod(endpoint, 'all', log, done), | ||
| byLocation: done => _getMetricsByLocation(endpoint, sites, requestMethod, log, done), |
| locationSchedules[loc] = new Date(val); | ||
| async.parallel( | ||
| { | ||
| states: done => _makeRequest(endpoint, statusPath, log, done), |
| async.parallel( | ||
| { | ||
| states: done => _makeRequest(endpoint, statusPath, log, done), | ||
| schedules: done => _makeRequest(endpoint, schedulePath, log, done), |
| getMDDiskUsage: cb => metadata.getDiskUsage(log, cb), | ||
| getDataDiskUsage: cb => data.getDiskUsage(log, cb), | ||
| getVersion: cb => getGitVersion(cb), | ||
| getObjectCount: cb => metadata.countItems(log, cb), |
| getDataDiskUsage: cb => data.getDiskUsage(log, cb), | ||
| getVersion: cb => getGitVersion(cb), | ||
| getObjectCount: cb => metadata.countItems(log, cb), | ||
| getCRRMetrics: cb => getCRRMetrics(log, cb), |
| getVersion: cb => getGitVersion(cb), | ||
| getObjectCount: cb => metadata.countItems(log, cb), | ||
| getCRRMetrics: cb => getCRRMetrics(log, cb), | ||
| getReplicationStates: cb => getReplicationStates(log, cb), |
| getObjectCount: cb => metadata.countItems(log, cb), | ||
| getCRRMetrics: cb => getCRRMetrics(log, cb), | ||
| getReplicationStates: cb => getReplicationStates(log, cb), | ||
| getIngestionInfo: cb => getIngestionInfo(log, cb), |
| getCRRMetrics: cb => getCRRMetrics(log, cb), | ||
| getReplicationStates: cb => getReplicationStates(log, cb), | ||
| getIngestionInfo: cb => getIngestionInfo(log, cb), | ||
| getVaultReport: cb => vault.report(log, cb), |
Convert reportHandler's _makeRequest, whose signature this change modified,
to an async function using the dual callback+async continuation trampoline:
callers keep the callback contract while the body is async/await. This
preempts the CodeQL js/callback-style-function alert on the function this
PR touched.
_makeRequest is an internal HTTP helper (endpoint, path, log, cb), not a
CORS-bearing lib/api handler, so the trampoline omits the
err.additionalResHeaders / collectCorsHeaders handling. request.get yields
two success values (response, body), which util.promisify cannot express, so
it is wrapped in a manual Promise. The (err, res) callback contract is
preserved exactly, including the cb('responseError', body) sentinel path.
The .then()/.catch() on the trampoline (eslint promise/prefer-await-to-then
and CodeQL js/promise-then-usage) are the accepted trade-off of this pattern.
Issue: CLDSRV-947
| return _makeRequest(endpoint, path, log) | ||
| .then(res => cb(null, res)) |
| if (error) { | ||
| return reject(error); | ||
| } | ||
| return resolve({ response, body }); |
There was a problem hiding this comment.
Why do we need response ?
There was a problem hiding this comment.
I mean with > 400, we'll have an error ?
There was a problem hiding this comment.
I legit can't wrap my head around this function lmao
sight
There was a problem hiding this comment.
I think its ok, looks like response is mostly because it was there before but yeah technically not used, probably ok to keep it, maybe _response ?
| } | ||
|
|
||
| const _makeRequest = (endpoint, path, cb) => { | ||
| const _makeRequest = (endpoint, path, log, cb) => { |
There was a problem hiding this comment.
nit: maybe makeRequest should take reqUuid directly 🤔
Forward the werelogs request-uid chain (
log.getSerializedUids()) on cloudserver's Backbeat report / CRR-metrics HTTP calls, via thex-scal-request-uidsheader.Previously
traceparentflowed on these calls but the reqUids chain did not. The header set here uses the same lowercase name cloudserver already reads on incoming admin requests (lib/server.js) and that arsenal'sRESTServerreads, so req_id continuity is preserved across the Backbeat metric and state/schedule calls.Scope: only the Backbeat metric/state calls that go through
reportHandler's_makeRequest(CRR/ingestion metrics, replication/ingestion states and schedules). The vault/metadata/data calls in reportHandler already forward reqUids through their arsenal clients and are unchanged.Issue: CLDSRV-947