feat(CLDSRV-884): Add OpenTelemetry tracing instrumentation#6140
Open
delthas wants to merge 9 commits into
Open
feat(CLDSRV-884): Add OpenTelemetry tracing instrumentation#6140delthas wants to merge 9 commits into
delthas wants to merge 9 commits into
Conversation
Contributor
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.3 #6140 +/- ##
===================================================
- Coverage 84.60% 84.51% -0.10%
===================================================
Files 206 210 +4
Lines 13322 13470 +148
===================================================
+ Hits 11271 11384 +113
- Misses 2051 2086 +35
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
…ring
Behind ENABLE_OTEL=true:
- NodeSDK with OTLP/HTTP exporter and ParentBased + ratio sampler
so we honor upstream NGINX/Beyla sampling decisions instead of
re-sampling them away.
- HTTP instrumentation with two hooks:
- ignoreIncomingRequestHook drops spans on probe / scrape paths
(/live, /ready, /_/healthcheck, /_/healthcheck/deep, /metrics)
and OPTIONS preflight.
- requestHook strips traceparent/tracestate on outbound requests
to hosts not in TRUSTED_HOSTS, so trace IDs do not leak to
external destinations. The client span is preserved (we still
observe the call) and tagged scality.trace.suppressed=true.
- buildTrustedHosts derives the allowlist from cloudserver Config
(vaultd, dataClient, metadataClient, bucketd, KMIP, KMS, scuba,
utapi, mongodb, hdclient/sproxyd connectors from locationConfig,
PUSH/MANAGEMENT_ENDPOINT env vars, plus loopback). A unit test
asserts every Config host shape is covered so the derivation
stays honest as new backends land.
- shutdownOtel() helper for the server's cleanUp() to await the
exporter flush before process.exit so in-flight traces are not
lost on SIGTERM.
- mongodb auto-instrumentation tuned for low cardinality; ioredis
enabled with requireParentSpan; fs / redis (v2/v3/v4) / aws-sdk
disabled.
When ENABLE_OTEL is unset the SDK and @opentelemetry/* packages are
not loaded at all - zero overhead off the OTEL path.
Issue: CLDSRV-884
Wire shutdownOtel() into the server's cleanUp() chain between closing HTTP servers and process.exit(0). Without this, async sdk.shutdown() fired by signal handlers can race the exit and lose buffered spans for whatever was still in flight at SIGTERM time. Inbound traceparent extraction is intentionally NOT done here: @opentelemetry/instrumentation-http already calls propagation.extract on every incoming request, creates a server span as a child of the remote parent, and sets that server span as the active context. A manual extract on top of that would replace the active context with the (non-recording) remote parent and demote downstream api spans to siblings - rather than children - of the HTTP server span, breaking the trace hierarchy in exactly the distributed-tracing scenarios the manual block was meant to support. Issue: CLDSRV-884
Add lib/instrumentation/simple.js exporting instrumentApiMethod, a wrapper that surrounds an S3 handler invocation with an OTEL span named api.<methodName>. The span owns the entire handler execution (auth, body parsing, metadata I/O, data path, finalizers) and becomes the parent for any auto-instrumentation spans (HTTP, MongoDB, ioredis) that fire underneath. Span name is the handler name verbatim - objectGetACL stays distinct from objectGet, objectPutTagging stays distinct from objectPut. ~70 handlers means ~70 distinct span names total, well within trace backend limits, and operators can tell variants apart without reading attributes. api.js applies the wrapper to every function-valued key in the api object except for the dispatcher (callApiMethod) and pure helpers (checkAuthResults, handleAuthorizationResults). New handlers added to the literal are automatically instrumented - no per-handler boilerplate to remember. The wrapper handles callback / promise / sync return paths, sets SpanStatusCode.OK or ERROR + recordException as appropriate, and sets cloudserver.error_code on the error path. When ENABLE_OTEL is unset @opentelemetry/api is not loaded and the wrapper returns the original function unchanged. Issue: CLDSRV-884
Temporarily point the arsenal dep at scality/Arsenal#improvement/ARSN-572/trace-context so we can validate end-to-end trace context propagation from cloudserver HTTP spans through to the MongoDB oplog on a test cluster. ARSN-572 adds traceContext plumbing on metadata writes; cloudserver needs no code change thanks to OTEL async context hooks. Yarn resolves the branch ref and pins the resolved commit hash in yarn.lock so installs are reproducible. Revert to a clean #8.x release tag once ARSN-572 ships. Issue: CLDSRV-884
…e filtering race-safe close()
delthas
force-pushed
the
improvement/CLDSRV-884/otel-instrumentation
branch
from
97b88f5 to
fb2ed26
Compare
| "@opentelemetry/sdk-trace-base": "^2.7.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.1", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#improvement/ARSN-572/trace-context", |
There was a problem hiding this comment.
Arsenal is pinned to a feature branch (improvement/ARSN-572/trace-context) instead of a release tag. The PR description notes this is temporary, but merging this means main carries an unpinned git dep — if the branch is force-pushed or rebased, installs become non-reproducible. Consider gating this PR behind the ARSN-572 arsenal release so it can land with a tag pin (e.g. #8.4.2), or at minimum add a tracking issue to ensure the branch pin is reverted before the next cloudserver release.
— Claude Code
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+65
to
+68
| return result.then( | ||
| value => { endSpan(); return value; }, | ||
| err => { endSpan(err); throw err; }, | ||
| ); |
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
4 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.
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.The four atomic commits:
feat: add OpenTelemetry tracing with trust boundaries and probe filteringParentBasedSampler({ root: TraceIdRatioBasedSampler(ratio) })so inboundtraceparentsampled=1from NGINX/Beyla is honored end-to-end (was being re-sampled by a plain ratio sampler before)auto-instrumentations-nodebundle (which pulled ~36 unused instrumentations: pg, mysql, kafkajs, cassandra, oracle, etc.):instrumentation-httpwithignoreIncomingRequestHook(drops k8s probes + OPTIONS) andrequestHook(stripstraceparent/tracestateon outbound requests to hosts not in the trusted allowlist; client span is preserved and taggedscality.trace.suppressed=true)instrumentation-mongodbwith low-cardinality settings (no collection names in span names, no captured query payloads)instrumentation-iorediswithrequireParentSpan: true(no orphan spans from background stats/rate-limit jobs)buildTrustedHosts(config)derives the allowlist from cloudserver's own Config (vaultd, dataClient, metadataClient, pfsClient, cdmi, bucketd, KMIP, KMS, scuba, utapi, mongodb replica set, backbeat, managementAgent +hdclient/sproxydbootstrap entries fromlocationConstraintsfor direct Scality storage connectors), plusPUSH_ENDPOINT/MANAGEMENT_ENDPOINTenv vars and loopback aliases. Unit test asserts every config host shape is included so the derivation stays honest.lib/tracing/healthPaths.js):/live,/ready,/_/healthcheck,/_/healthcheck/deep,/metricsproduce no spansdiag.setLogger(new DiagConsoleLogger(), DiagLogLevel.WARN)so SDK errors (export failures, malformedtraceparent, etc.) surface instead of disappearingattributeValueLengthLimit: 4096,attributeCountLimit: 128,eventCountLimit: 128,linkCountLimit: 128to bound memory under pathological caseslogRecordProcessors: [],metricReaders: []— explicit traces-only; without these, NodeSDK silently spins up OTLP log + metric exportersfeat: flush OTEL on shutdownshutdownOtel()wired intoS3Server.cleanUp()'s shutdown chain (betweenserver.close()andprocess.exit(0)), capped at 5 s viaPromise.raceso an unreachable collector can't block past Kubernetes' default 30 sterminationGracePeriodSecondscaughtExceptionShutdown()also attempts a best-effort flush beforeprocess.exit(1)on the non-cluster path.finally()guard so any unexpected promise rejection in the shutdown chain still exits the process@opentelemetry/instrumentation-httpalready extractstraceparentand creates the server span as a child of the remote parent. A manual extract on top would replace the active context with the (non-recording) remote parent and demote downstreamapi.<handler>spans to siblings instead of children of the HTTP server span. Removed from the original draft.feat: instrument all S3 API handlers with OTEL spanslib/instrumentation/simple.jsexports a single helperinstrumentApiMethod(handler, methodName)that wraps an S3 handler in anapi.<methodName>span owning the entire handler execution (auth, body parsing, metadata I/O, data path, finalizers). Auto-instrumentation spans (HTTP, MongoDB, ioredis) nest beneath. ~70 distinct span names total — well within trace-backend limits.lib/api/api.jsapplies the wrapper viaObject.entries(api)loop with aNON_HANDLER_KEYSskip set (callApiMethod,checkAuthResults,handleAuthorizationResults). New S3 handlers added to the literal are auto-instrumented; no per-handler boilerplate.cloudserver.error_codeon the error pathchore: bump arsenal to ARSN-572 PoC branch for e2e trace context testingtraceContextplumbing to MongoDB metadata writes (object oplog entries carry the trace context that caused them, so async consumers — Backbeat, Sorbet, lifecycle — can continue the trace across the oplog/queue boundary)8.3.xrelease tag once ARSN-572 shipsOrigin
Extracted and cleaned up from William Lardier's
user/test/wlardier/servicemesh-2branch (based ondevelopment/9.0, July–Sep 2025), which mixed OTEL with performance optimizations and debug artifacts. This PR contains only the OTEL instrumentation, rebased ontodevelopment/9.3. Dropped:lib/otelContextPropagation.js(dead code; never imported)MOCK_DOAUTH/lib/api/apiUtils/mock/backendMocks.js(production-dangerous auth-result caching, unrelated to tracing)console.log()debug artifacts, commented-out span codemonitorLatency,releaseRequestContexts, arsenal perf pin) — out of scope; will land separately if neededTests
29 unit tests (22 in
tests/unit/lib/otel.spec.js, 7 intests/unit/lib/instrumentationSimple.spec.js) coveringextractHost,buildTrustedHosts(full config snapshot + locationConstraints connector enumeration),makeRequestHook(outbound trust check, inbound IncomingMessage skip),isHealthPath, and the full lifecycle ofinstrumentApiMethod(callback success/error, double-end guard, async resolve/reject, sync throw, OTEL-disabled bypass).Context
@opentelemetry/instrumentation-http's automatic outboundtraceparentpropagationIssue: CLDSRV-884