feat(CLDSRV-884): Add OpenTelemetry tracing instrumentation#6140
feat(CLDSRV-884): Add OpenTelemetry tracing instrumentation#6140delthas wants to merge 8 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. |
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6140 +/- ##
===================================================
+ Coverage 85.21% 85.27% +0.05%
===================================================
Files 207 211 +4
Lines 13832 13972 +140
===================================================
+ Hits 11787 11914 +127
- Misses 2045 2058 +13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
delthas
force-pushed
the
improvement/CLDSRV-884/otel-instrumentation
branch
from
9c93a1d to
97e63fc
Compare
delthas
force-pushed
the
improvement/CLDSRV-884/otel-instrumentation
branch
from
97e63fc to
f978280
Compare
delthas
force-pushed
the
improvement/CLDSRV-884/otel-instrumentation
branch
from
f978280 to
06eea4e
Compare
delthas
force-pushed
the
improvement/CLDSRV-884/otel-instrumentation
branch
from
06eea4e to
75c3afb
Compare
delthas
force-pushed
the
improvement/CLDSRV-884/otel-instrumentation
branch
from
75c3afb to
207b785
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
Pre-existing prettier debt on lib/api/api.js and lib/server.js — the 9.4 base did not run prettier on these. Extracted as a separate commit so the subsequent CLDSRV-884 OTEL commits show only logic changes, not formatting noise. Issue: CLDSRV-884
|
Updates since the last review pass:
|
|
Brings in the OTEL Node SDK, OTLP/HTTP exporter, and the three instrumentation packages we explicitly opt into (http, mongodb, ioredis). Split out of the main CLDSRV-884 work so reviewers can verify the dependency surface independently of the code that consumes it. Issue: CLDSRV-884
Pure helper: parses OTEL_TRUSTED_HOSTS into a Set, exposes a requestHook that strips traceparent/tracestate from outbound HTTP requests to hosts outside the set. Loopback aliases are always trusted so dev / local tooling work without extra config. Used in the next commit's bootstrap (HttpInstrumentation requestHook). Issue: CLDSRV-884
Pure helper: matches request URLs against a set of known probe/scrape paths (k8s liveness/readiness, metrics scrape). Wired into HttpInstrumentation.ignoreIncomingRequestHook in the next commit so probe traffic doesn't dominate the trace volume. Issue: CLDSRV-884
The two pieces that wire OTEL into the process:
- lib/tracing/index.js: init() / close() / isEnabled() facade.
Boots the Node SDK with an explicit allowlist of three
instrumentations (http, mongodb, ioredis), a ParentBased
TraceIdRatio sampler, the trust-boundary requestHook, and
the health-probe ignore hook. Cluster primary is skipped
(workers re-exec the entry and init themselves). close()
flushes the BatchSpanProcessor with a bounded deadline so
SIGTERM can't hang past the kubelet's grace period.
- lib/instrumentation/simple.js: instrumentApiMethod() — wraps
a callback-style or async S3 handler in a span that ends
when the underlying handler settles. The next commits use
this to instrument all 80+ S3 API methods.
- index.js: calls tracing.init() before requiring server.js
so the require-in-the-middle patches land before any
HTTP/Mongo/ioredis module is loaded.
Issue: CLDSRV-884
|
@DarkIsDude ready for review. For easier review, ignore the prettier commit: https://github.com/scality/cloudserver/pull/6140/changes/837c50f2b91d2a391ba1266761de74a17230e87c..396404d3f22623c06bc924f9669d431964983555 |
| // always-on sampling overwhelms the exporter and storage with traffic | ||
| // nobody queries. | ||
| const HEALTH_PATHS = new Set(['/live', '/ready', '/_/healthcheck', '/_/healthcheck/deep', '/metrics']); | ||
|
|
There was a problem hiding this comment.
should health path filtering be configurable, i.e. allow enabling it (possibly with an extra env variable)?
There was a problem hiding this comment.
simple.js is a really weird name: can we find better?
Not much idea if it is in its own module/folder except instrumentatin/instrumentatin.js); maybe tracing/instrumentation.js would be better ?
There was a problem hiding this comment.
since both module are inter-dependent (tracing requires this one & vice-versa), they should probably indeed go into the same module...
| } | ||
|
|
||
| function instrumentCallbackHandler(self, apiMethod, spanName, args, callbackIndex) { | ||
| const api = require('@opentelemetry/api'); |
There was a problem hiding this comment.
is there any penalty with repeatedly require'ing the same api module (i.e. extra load time on startup, more memory usage...), or is this somehow memoized by node.js anyway and thus essentially free (but for the syntaxic weight, which stems from line 15) ?
There was a problem hiding this comment.
Alternatively, this may be done in api.js or at the top level of module:
const OTEL_ENABLED = process.env.get('OTEL_ENABLED');
const api = OTEL_ENABLED ? require('api') : null;this would avoid the extra api parameter in endSpan, and repeated require/api var ; while still not loading OTEL module unless enabled.
There was a problem hiding this comment.
About the repeat require, it's a no cost. Node have a cache
| for (const [name, handler] of Object.entries(api)) { | ||
| if (typeof handler === 'function' && !NON_INSTRUMENTED_KEYS.has(name)) { | ||
| api[name] = instrumentApiMethod(handler, name); | ||
| } |
There was a problem hiding this comment.
a really dumb question: what do we get by instrumenting each method here, instead of hooking just once in Server.routeRequest (in server.js) or in arsenal's router ?
| const { config } = require('../Config'); | ||
| const cluster = require('cluster'); |
There was a problem hiding this comment.
can also conditinally require:
const cluster = process.env.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT ? require('cluster') : undefined;(and specifically for cluster, I think it is loaded anyway by server.js ?)
| const { config } = require('../Config'); | ||
| const cluster = require('cluster'); |
There was a problem hiding this comment.
(c.f. other comment: lazy loading opentelemetry here is acceptable i guess, as this is very limited in scope: to a single functino; but more problematic in instrumentatin where all functions use it...)
| if (process.env.OTEL_SAMPLING_RATIO !== undefined) { | ||
| const parsed = Number(process.env.OTEL_SAMPLING_RATIO); | ||
| if (!Number.isFinite(parsed) || parsed < 0 || parsed > 1) { | ||
| throw new Error( |
There was a problem hiding this comment.
nit: in cloudserver' Config.js, we assert() when config is valid. Is it better to throw or to assert ?
| // worker process with 'SIGTERM'. | ||
| // worker.kill() is graceful (closes servers, disconnects IPC) but | ||
| // does not fire our SIGTERM handler, so the BatchSpanProcessor | ||
| // would lose buffered spans without an explicit flush here. |
There was a problem hiding this comment.
should we/can we add tests for verify behavior on shutdown?
DarkIsDude
left a comment
DarkIsDude
left a comment
There was a problem hiding this comment.
Almost good for me. My main concern is about the package.json 🙏
| "ts-morph/**/brace-expansion": "^5.0.5", | ||
| "ts-morph/**/picomatch": "^4.0.4" | ||
| "ts-morph/**/picomatch": "^4.0.4", | ||
| "protobufjs": "^7.5.8" |
There was a problem hiding this comment.
Why this one is needed now ?
| // Start tracing before requiring anything that hooks into HTTP, MongoDB, | ||
| // or ioredis — instrumentation patches modules on require, so anything | ||
| // loaded earlier than init() would run unpatched. |
There was a problem hiding this comment.
| // Start tracing before requiring anything that hooks into HTTP, MongoDB, | |
| // or ioredis — instrumentation patches modules on require, so anything | |
| // loaded earlier than init() would run unpatched. |
This is purely standard behaviour in tracing / monitoring. All package that do that (datadog, optl, newrelic, ...) are doing the same things and explain it already.
| } | ||
|
|
||
| function instrumentCallbackHandler(self, apiMethod, spanName, args, callbackIndex) { | ||
| const api = require('@opentelemetry/api'); |
There was a problem hiding this comment.
About the repeat require, it's a no cost. Node have a cache
5 participants
Summary
Add OpenTelemetry tracing instrumentation to cloudserver, gated behind
ENABLE_OTEL=true. When the flag is unset, no@opentelemetry/*package is loaded — zero overhead off the OTEL path.Rebased onto
development/9.4. The branch leads with a prettier-only commit so the feature commits show logic changes, not formatting noise.Commits
chore: prettier-format files touched by CLDSRV-884— isolates the pre-existing prettier reformat oflib/api/api.js+lib/server.jsso the subsequentfeat:commits carry only logic deltas (e.g. the api.js instrumentation is then +11 / -0).chore: add OpenTelemetry dependencies— the OTEL Node SDK, OTLP/HTTP trace exporter, and the three instrumentation packages we explicitly opt into (instrumentation-http,instrumentation-mongodb,instrumentation-ioredis), all as direct deps — noauto-instrumentations-nodebundle (which would pull ~36 unused instrumentations).protobufjsresolution for a transitive CVE.feat: add trust-boundary host filter(lib/tracing/trustedHosts.js) — outbound trust boundary.OTEL_TRUSTED_HOSTSis an operator-supplied comma-joined list of lowercase bare hostnames;loadTrustedHosts()splits it and adds loopback aliases (localhost,127.0.0.1,::1).makeRequestHookstripstraceparent/tracestateon outbound requests to hosts not in the set (the client span is preserved and taggedscality.trace.suppressed=true).normalizeHostHeaderstrips IPv6 brackets + port from the incomingHostheader before matching. Cloudserver does not self-derive the list from its Config — that is computed operator-side (tracked as ZKOP-551); unset env → only loopback trusted (safe default).feat: add health-probe ignore filter(lib/tracing/healthPaths.js) —isHealthPath()matches k8s liveness/readiness + metrics-scrape paths; wired intoinstrumentation-http'signoreIncomingRequestHook(which also dropsOPTIONSpreflight) so probe traffic produces no spans.feat: add OTEL bootstrap and manual span helperlib/tracing/index.js—init()/close()/isEnabled()facade. Boots NodeSDK with the OTLP/HTTP exporter,ParentBasedSampler({ root: TraceIdRatioBasedSampler(ratio) })(so an inboundtraceparentwithsampled=1from NGINX/Beyla is honored end-to-end), the three instrumentations above, span limits, andlogRecordProcessors: []/metricReaders: [](explicit traces-only — otherwise NodeSDK silently starts OTLP log + metric exporters).OTEL_SAMPLING_RATIOis parsed withNumber()and fails fast outside[0,1]. Cluster primary skips init (workers re-exec and init themselves). All@opentelemetry/*requires are lazy insideinit()so a process with OTEL off never loads them.lib/instrumentation/simple.js—instrumentApiMethod(handler, methodName)wraps an S3 handler in anapi.<methodName>span owning the whole handler execution; auto-instrumentation spans (HTTP/Mongo/ioredis) nest beneath. Handles callback / Promise / sync return shapes with atry/catch+ rethrow (no.then()chains — repo CodeQL rule), and setscloudserver.error_codeon the error path.index.jscallsrequire('./lib/tracing').init()beforeserver.js, sorequire-in-the-middlepatches land before any HTTP/Mongo/ioredis module is loaded.feat: flush OTEL on shutdown—S3Server.cleanUp()is now async: itawaitstracing.close()afterserver.close()(viautil.promisify) and beforeprocess.exit(0).close()is race-safe (captures + nulls the module-level SDK before awaiting) and bounded at 5 s viaPromise.racewith a.unref()'d timer, so an unreachable collector can't block past Kubernetes' default 30 s grace period; the shutdown promise carries its own rejection handler so a late failure can't crash the process.caughtExceptionShutdown()flushes on the non-cluster (process.exit(1)) and cluster-worker (worker.kill()) paths; the cluster primary keeps its original survive-and-log behavior.instrumentation-httpalready extractstraceparentand makes the server span a child of the remote parent. A manual extract would demote downstreamapi.<handler>spans to siblings.feat: instrument all S3 API handlers with OTEL spans—lib/api/api.jsapplies the wrapper via anObject.entries(api)loop with aNON_INSTRUMENTED_KEYSskip set (callApiMethod,checkAuthResults,handleAuthorizationResults). New S3 handlers are auto-instrumented; no per-handler boilerplate. Pure logic delta is 11 lines.chore: bump arsenal to ARSN-572 PoC branch for e2e trace context testing— temporary: pins arsenal at the ARSN-572 branch (scality/Arsenal#2611), which stampstraceContextonto MongoDB metadata writes so async consumers (Backbeat, Sorbet, lifecycle) can continue the trace across the oplog boundary. To be repinned to a real release tag once ARSN-572 ships.Origin
Extracted and cleaned up from William Lardier's
user/test/wlardier/servicemesh-2branch (OTEL mixed with perf optimizations + debug artifacts). This PR contains only the OTEL instrumentation. Dropped:lib/otelContextPropagation.js(dead code),MOCK_DOAUTHauth-result caching, ~15console.logartifacts, and all performance optimizations (out of scope).Tests
34 unit tests, no functional regressions:
tests/unit/lib/tracing/init.spec.js(10) —isEnabled,init(no-op when off, throws without endpoint, boots + idempotent — asserted viasinonstubs onNodeSDK.prototype.start),close(no-op, post-init, exactly-onceshutdownunder concurrent callers)tests/unit/lib/tracing/trustedHosts.spec.js(13) —loadTrustedHostsparsing + loopback defaults;makeRequestHooktrusted/untrusted/inbound-skip/missing-host, port + IPv6-bracket Host-header normalizationtests/unit/lib/tracing/healthPaths.spec.js(4) — probe-path matchingtests/unit/lib/instrumentationSimple.spec.js(7) —instrumentApiMethodcallback success/error, sync throw, async resolve/reject, sync return, OTEL-off bypassContext
instrumentation-http's automatic outboundtraceparentpropagationIssue: CLDSRV-884