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 14 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.
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This file should not be committed to the feature branch.
- Pass httpAgentOptions to OTLP exporter config as agentOptions - Add debug logging to verify cert is being passed - Remove monkey-patching of https.request (didn't work due to OTEL instrumentation)
The OTLP HTTP exporter expects 'httpAgentOptions' in the config, not 'agentOptions'. This was causing the cert/key to be ignored.
When config.url is provided, OTLP exporter uses it as-is without appending the signal resource path. CaaS binding provides base URL without path, so we need to append /v1/traces and /v1/metrics.
SummaryThe following content is AI-generated and provides a summary of the pull request: Add
|
There was a problem hiding this comment.
The PR adds the telemetry-to-caas kind with reasonable structure, but there are several correctness issues that need attention: the shared OTEL_EXPORTER_OTLP_ENDPOINT env var is clobbered by both signal exporters (traces and metrics overwrite each other), invalid base64 mTLS data is silently accepted before failing at TLS handshake time, and an unsanitised user-provided regex pattern can crash the process at startup.
PR Bot Information
Version: 1.26.11
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review - Correlation ID:
66d6ae55-ae8a-46ec-9f81-f076100fcf4a - File Content Strategy: Full file content
| try { | ||
| const cert = Buffer.from(mtlsCreds.cert, 'base64').toString('utf-8') | ||
| const key = Buffer.from(mtlsCreds.key, 'base64').toString('utf-8') | ||
|
|
||
| // Store the mTLS options for the exporter's httpAgentOptions | ||
| // The OTLP HTTP exporter will create an https.Agent with these options | ||
| credentials.httpAgentOptions = { | ||
| cert: cert, | ||
| key: key, | ||
| keepAlive: true | ||
| } | ||
| } catch (err) { | ||
| LOG._error && LOG.error('Failed to configure CaaS mTLS:', err.message) | ||
| } |
There was a problem hiding this comment.
Bug: Buffer.from(..., 'base64').toString('utf-8') does not throw on invalid base64 input — it silently produces garbage bytes. If the mTLS setup error is then swallowed by the catch, the exporter will be configured with a corrupt certificate and will fail at connection time with a cryptic TLS error instead of a clear startup message.
| try { | |
| const cert = Buffer.from(mtlsCreds.cert, 'base64').toString('utf-8') | |
| const key = Buffer.from(mtlsCreds.key, 'base64').toString('utf-8') | |
| // Store the mTLS options for the exporter's httpAgentOptions | |
| // The OTLP HTTP exporter will create an https.Agent with these options | |
| credentials.httpAgentOptions = { | |
| cert: cert, | |
| key: key, | |
| keepAlive: true | |
| } | |
| } catch (err) { | |
| LOG._error && LOG.error('Failed to configure CaaS mTLS:', err.message) | |
| } | |
| const certRaw = Buffer.from(mtlsCreds.cert, 'base64').toString('utf-8') | |
| const keyRaw = Buffer.from(mtlsCreds.key, 'base64').toString('utf-8') | |
| if (!certRaw.includes('-----BEGIN') || !keyRaw.includes('-----BEGIN')) { | |
| throw new Error('Decoded mTLS cert or key does not appear to be valid PEM data') | |
| } | |
| // Store the mTLS options for the exporter's httpAgentOptions | |
| // The OTLP HTTP exporter will create an https.Agent with these options | |
| credentials.httpAgentOptions = { | |
| cert: certRaw, | |
| key: keyRaw, | |
| keepAlive: true | |
| } | |
| } catch (err) { | |
| LOG._error && LOG.error('Failed to configure CaaS mTLS:', err.message) | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| if (!credentials) throw new Error('No CaaS credentials found. Make sure the app is bound to a caas-service instance.') | ||
| augmentCaaSCreds(credentials) | ||
| // Set OTLP env vars to binding credentials (prevents stale env values from taking precedence) | ||
| process.env.OTEL_EXPORTER_OTLP_ENDPOINT = credentials.baseUrl |
There was a problem hiding this comment.
Bug: process.env.OTEL_EXPORTER_OTLP_ENDPOINT is set to credentials.baseUrl (the path-less base URL) even though config.url is immediately set to the full /v1/traces URL. This env var mutation persists for the entire process lifetime and will be read by any subsequently initialised OTLP exporter (e.g. the metrics exporter called right after this), overwriting it again with the metrics-scoped base URL and potentially causing both exporters to send to the wrong endpoint. Consider removing this process.env mutation, or at minimum using the signal-specific env var (OTEL_EXPORTER_OTLP_TRACES_ENDPOINT).
| process.env.OTEL_EXPORTER_OTLP_ENDPOINT = credentials.baseUrl | |
| process.env.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT = credentials.baseUrl + '/v1/traces' | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| if (!credentials) throw new Error('No CaaS credentials found. Make sure the app is bound to a caas-service instance.') | ||
| augmentCaaSCreds(credentials) | ||
| // Set OTLP env vars to binding credentials (prevents stale env values from taking precedence) | ||
| process.env.OTEL_EXPORTER_OTLP_ENDPOINT = credentials.baseUrl |
There was a problem hiding this comment.
Bug: Same process-wide env var mutation as in tracing/index.js — OTEL_EXPORTER_OTLP_ENDPOINT is set to the base URL without a signal-specific path, which conflicts with the value set by the tracing exporter (or vice-versa depending on initialisation order). Use OTEL_EXPORTER_OTLP_METRICS_ENDPOINT instead.
| process.env.OTEL_EXPORTER_OTLP_ENDPOINT = credentials.baseUrl | |
| process.env.OTEL_EXPORTER_OTLP_METRICS_ENDPOINT = credentials.baseUrl + '/v1/metrics' | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const vcap = JSON.parse(process.env.VCAP_SERVICES) | ||
| // Look for user-provided service with name matching configured pattern | ||
| const { mtls_service_pattern } = cds.env.requires.telemetry | ||
| if (!mtls_service_pattern) return |
There was a problem hiding this comment.
Logic Error: getCredsForCaaSMtls() reads cds.env.requires.telemetry.mtls_service_pattern but getCredsForCaaSMtls is only reached via augmentCaaSCreds, which is only called when kind matches to-caas. At that point cds.env.requires.telemetry is the merged kind config, so mtls_service_pattern should be present — but if for any reason the property is absent (e.g. user overrides the kind config without it), the function silently returns undefined and the else branch only logs a warning. The mTLS credentials silently become unconfigured rather than raising a clear error.
| if (!mtls_service_pattern) return | |
| if (!mtls_service_pattern) { | |
| LOG._warn && LOG.warn('CaaS: mtls_service_pattern is not configured; cannot locate mTLS credentials.') | |
| return | |
| } | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| // Look for user-provided service with name matching configured pattern | ||
| const { mtls_service_pattern } = cds.env.requires.telemetry | ||
| if (!mtls_service_pattern) return | ||
| const pattern = new RegExp(mtls_service_pattern, 'i') |
There was a problem hiding this comment.
Bug: mtls_service_pattern is taken verbatim from user-supplied configuration and passed directly into new RegExp(...) without any escaping. A malformed pattern (e.g. "caas-mtls(") will throw a SyntaxError that propagates up uncaught and crashes the process during startup.
| const pattern = new RegExp(mtls_service_pattern, 'i') | |
| let pattern | |
| try { | |
| pattern = new RegExp(mtls_service_pattern, 'i') | |
| } catch (e) { | |
| LOG._error && LOG.error(`CaaS: Invalid mtls_service_pattern "${mtls_service_pattern}":`, e.message) | |
| return | |
| } | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
2 participants
Note: CaaS requires mTLS authentication with SAP-signed certificates. The certificate must be obtained separately via BTP Certificate Service.