Add telemetry-to-caas kind for CaaS (Collector as a Service) support - #436
Add telemetry-to-caas kind for CaaS (Collector as a Service) support#436vkozyura wants to merge 79 commits into
Conversation
- Add telemetry-to-caas kind definition in package.json - Add getCredsForCaaS() to extract credentials from caas-service binding - Add augmentCaaSCreds() to configure OTLP endpoint URL - Handle CaaS in tracing and metrics exporters Note: CaaS requires mTLS authentication with SAP-signed certificates. The certificate must be obtained separately via BTP Certificate Service.
There was a problem hiding this comment.
The PR is generally well-structured, but has one logic bug: when only a gRPC OTLP endpoint is present in the CaaS binding, credentials.url is set to undefined (the falsy http value), silently breaking the exporter. Please address the flagged issues before merging.
PR Bot Information
Version: 1.26.5
- Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
be7942dc-b11a-457c-a83d-2aafac5eb2c3 - File Content Strategy: Full file content
This file should not be committed to the feature branch.
4f2f45d to
1906cd7
Compare
| const cds = require('@sap/cds') | ||
| const LOG = cds.log('telemetry') | ||
|
|
||
| const MAX_BUFFER_SIZE = 1000 |
There was a problem hiding this comment.
let ai calculate it :)
MAX_BUFFER_SIZE = 1000 buffers batches, not items:
| Signal | Batch Size | Item Size | 1000 Batches |
|---|---|---|---|
| Traces | 512 spans | ~2 KB | ~1 GB |
| Metrics | varies | ~1 KB | ~100 MB |
| Logs | 512 records | ~1 KB | ~500 MB |
This is way too much. The ZTI window is typically seconds. 10-50 batches is plenty
solution: changed to 10 (16MB in worst case)
| const ztiAgentFactory = createZTIAgentFactory() | ||
| if (ztiAgentFactory) { | ||
| credentials.httpAgentOptions = ztiAgentFactory | ||
| credentials.useZTI = true | ||
| return | ||
| } | ||
|
|
||
| const staticAgentFactory = createStaticAgentFactory() | ||
| if (staticAgentFactory) { | ||
| credentials.httpAgentOptions = staticAgentFactory | ||
| return | ||
| } |
There was a problem hiding this comment.
the agent factory gets called every time a new connection is opened, correct? in that case, i think we don't need this distinction here. that agent factory should just always use cds.env.requires.telemetry.x509, which gets updated by the svid watcher in case of ztis.
There was a problem hiding this comment.
The agent factory is called once, not per-Connection http-exporter-transport.js:23-34 and cached for the process lifetime.
There was a problem hiding this comment.
ok. still the agent factory should be something like:
() => {
const { cert, key } = cds.env.requires.telemetry.x509
const agent = new https.Agent({ cert, key, keepAlive: true })
cds.on('svid', ({ cert, key }) => Object.assign(agent.options, { cert, key }))
return agent
}the watcher will not remain in telemetry. other services will need to support as well. the more we decouple now the better. question is how to handle missing cert during startup...
There was a problem hiding this comment.
instead of a lazy exporter, could we create the standard exporter and temporarily swap their export function with one that buffers, maybe even using the file system to reduce memory load?
There was a problem hiding this comment.
still need buffer logic somewhere if first span completes and triggers export() and SVID files don't exist yet - swapping export function is too hacky for me :( and using file system looks like overkill for a seconds-long window. i reduced buffersize to 10 - enough for few seconds and only 16MB in worst scenario.
There was a problem hiding this comment.
imho, swapping the export function is less hacky than the lazy exporter. or at least less risky. as demonstrated by the temporality bug. export is a single, clearly defined function.
sjvans
left a comment
There was a problem hiding this comment.
Thanks @vkozyura — the ZTI rework cleared all the earlier bot/CodeQL findings (the shared-env clobbering, base64/PEM handling, the unescaped RegExp, the URL-substring checks). A few things left before this can go into 2.1.0.
Blocking
1. CaaS metrics silently downgrade DELTA → CUMULATIVE on the ZTI path
createCaaSExporter returns a LazyExporter (lib/exporter/LazyExporter.js), which only exposes export/shutdown/forceFlush. PeriodicExportingMetricReader looks for exporter.selectAggregationTemporality, doesn't find it on the wrapper, and falls back to the default CUMULATIVE selector — so the temporalityPreference ??= DELTA we set on the inner OTLP exporter is ignored. Every other kind in this repo emits DELTA. static-x509 returns the real exporter directly so it's fine there; this only bites the (recommended) ZTI path, and since no test covers CaaS metrics it went unnoticed. I'd rather fix the root cause than proxy one more method — see design note B.
2. try/catch around the logs exporter still masks setup errors (lib/logging/index.js:38-44)
Same point as my earlier comment — it swallows a missing/broken logs-exporter module into null (logging silently disabled) for all kinds, inconsistent with tracing/metrics which still fail loudly via _require. Please drop it.
Design — two components reinvent platform machinery
Root fact worth stating up front: the OTLP http transport calls the httpAgentOptions factory exactly once and caches the returned Agent for the process lifetime (@opentelemetry/otlp-exporter-base → http-exporter-transport.js _loadUtils). Two consequences:
A. Cert handling — one source of truth + one rotation mechanism.
Today there are two parallel cert paths: createStaticAgentFactory closure-captures the decoded x509 once (so static certs never rotate on renewal), and createZTIAgentFactory → zti.js reimplements file reads plus _cached/_paths/_rotatingAgent module globals plus an fs.watch+debounce. Let's collapse this: an SVID watcher keeps cds.env.requires.telemetry.x509 current, and a single rotating agent sources cert/key from there and destroy()s its sockets on change. Note rotation still needs the self-refreshing agent + destroy() — because OTel caches the Agent, merely reading x509 inside the factory wouldn't rotate. The win is the single source of truth, unifying the static and ZTI paths, and static certs becoming rotatable for free.
B. LazyExporter — override, don't replace.
Because the agent factory is only invoked on the first send(), the OTLP exporter can be constructed before the SVID files exist — so the "defer construction until certs are ready" premise mostly doesn't hold. Prefer constructing the real exporter, and if we truly can't lose the first-export window, override just its export (FS-backed if we want crash durability) so the full exporter interface — including the temporality selector from blocker #1 — stays intact. A partial hand-rolled stand-in will keep leaking methods; this is exactly how #1 happened. The 1000-item in-memory drop-oldest ring also duplicates what BatchSpanProcessor/PeriodicExportingMetricReader already do and loses everything on restart.
Minor
package.jsonstill declaresmtls_service_pattern— nothing reads it anymore (leftover from the removedgetCredsForCaaSMtls). Remove.zti.jsrequireshttpsat module top level — per OTel's own guidance for agent factories, load it lazily (ascreateStaticAgentFactorydoes) so it can't preempt@opentelemetry/instrumentation-httppatching, now that #475 enabled HTTP instrumentation.- CHANGELOG: use
## Version 2.1.0 - tbd(our convention for the unreleased section). - The
BatchLogRecordProcessor({ exporter })/SimpleLogRecordProcessor({ exporter })change is correct for@opentelemetry/sdk-logs@0.221but touches all kinds, not just CaaS — worth splitting into its own PR.
Before it can ride 2.1.0
- Retarget to
develop— main-based PRs re-trigger the release ancestry problem. - Port
test/caas.test.jsoff jest —developis on vitest (#474);jest.fn/jest.isolateModuleswon't resolve there. - Add a CaaS metrics test (would have caught blocker #1) and assert the signal-suffixed exporter URL (
baseUrl + '/v1/traces'etc.) against a real exporter — the in-memory exporter ignoresconfig.url, so that path is currently unverified.
|
the SVID watcher should emit an event on
crash durability is not the point, minimized memory usage is |
I went with the quick fix (Option A) to unblock the PR |
removed |
"zti.js only extends https.Agent to hold certificates — it doesn't make any HTTP requests itself |
Added @opentelemetry/sdk-logs: >=0.221 as optional peerDependency — npm will warn if users have an older incompatible version, but won't require it if logging isn't used |
which option a? |
for now #436 (comment). we still can discuss emitting an event as future work. bli`? |
there's no rush, we can spend the time now. |
postpone until the content is clarified |
it may or may not make a difference, still we should follow the instruction |
already solved on develop: telemetry/lib/logging/index.js Line 163 in 48c9934 |
Note: CaaS requires mTLS authentication with SAP-signed certificates. The certificate must be obtained separately via BTP Certificate Service.