mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
Ignacy Łątka
16ad44bb0e
Two test files that look at what #570's exporter actually *does*, plus the configuration holes they exposed. The suite on #570 proves the exporter is **configured** right - the url, the headers, the timeouts, all read off the constructor. Nothing looked at the request that comes out of it, or at what happens when the collector does not answer. Both gaps are invisible from inside Argent by design: a failed export never reaches the caller, and an OTLP exporter counts any 2xx as delivered. | | | |---|---| | **`otel-wire.test.ts`** | Captures the real exporter's request off a loopback socket: uncompressed OTLP/JSON on `POST /v1/logs`, where each field the ingestion schema reads actually sits, the batch-size cap, and connection reuse. | | **`otel-unreachable.test.ts`** | Drives the same exporter at a silent socket, a refused port, a 503, a 401 and a 204, and asserts each drain finishes inside its deadline and what the collector really received. | | **`src/otel.ts`** | Two holes the tests exposed: `compression` and the six OTLP certificate variables are now settled in code instead of left to the environment. | **No docs update needed** - no user-facing capability, CLI surface, config key, flow file or MCP tool changed. <details> <summary><b>The two source fixes</b></summary> **`compression` was the one OTLP knob still left to the environment.** With `OTEL_EXPORTER_OTLP_COMPRESSION=gzip` (or its `_LOGS_` variant) set - and a machine already running OpenTelemetry has it set for its own collector - argent gzips its batches. Nothing downstream notices: a gzipped 2xx is a delivered batch, while the request this suite and the deploy-time smoke test are written against quietly stops being the request argent sends. `createExporter` now passes `compression` explicitly, which beats both variables. Unlike the header channel, where the SDK *merges* whatever the code does not set and the variables have to be cleared outright, one explicit value settles this. **The certificate variables could hang the command outright.** The SDK resolves `OTEL_EXPORTER_OTLP_{,LOGS_}{CERTIFICATE,CLIENT_CERTIFICATE,CLIENT_KEY}` with a synchronous `fs.readFileSync` while the exporter is constructed, and only then discards the https agent it built from them in favour of the explicit `httpAgentOptions`. The file is read for nothing - and a path that never answers hangs the command that emitted the event, uninterruptibly, on `getClient()`'s path. Measured with a fifo that has no writer: `createExporter` never returned (killed at 12s); with the variables cleared, 1.0 ms. They join the header pair in the list cleared across the constructor and restored on the way out. Some source prose was also wrong about the SDK and is corrected: it does **not** reject a null attribute - it serializes one as an empty OTLP value, `{"key":"cloud_agent","value":{}}` with `droppedAttributesCount: 0`, stored as and unrecoverable from a property that really was empty. That is the actual reason `toAttributes` drops the key. </details> <details> <summary><b>What the tests pin, and what they deliberately do not</b></summary> The wire test **mirrors** `OtelClient.emit` rather than calling it: `getClient()` resolves its endpoint from the hard-coded `OTLP_LOGS_ENDPOINT`, and that being unredirectable is the anti-exfiltration property the client is meant to have, so there is deliberately no seam to aim it at a test server. `otel-endpoint.test.ts` covers the other half against the real emit path - `SERVICE_NAME`, the logger name, event-name-as-body, the dropping of null/undefined properties - so a rename is caught there rather than in the mirror. What enforces the drain budgets is the exporter's `timeoutMillis`, *not* the processor's `exportTimeoutMillis`, which `forceFlush()` awaits straight past. Measured against a silent socket: `exportTimeout` 200 / `timeoutMillis` 5000 drains in 5012 ms, the reverse in 202 ms. 503 is the one collector state where the batch is sent twice, and the request count is the only thing that tells a retried failure from a rejected one - the SDK reports the class, not the code. The retry itself is only reachable while the next backoff fits in what is left of `timeoutMillis`, so the jitter is pinned to its low draw and the assertion turns on the round trip alone. `httpAgentOptions.timeout` is the one option with no behavioural coverage: it covers a socket stuck in *connect*, which needs an address whose packets are dropped and is not portably reproducible here. It stays pinned at the constructor and the docstring says so. Its `keepAlive` half **is** covered, on the two batches of a single drain. Real deadlines cost real time: `otel-unreachable.test.ts` waits ~5.1 s, taking the package suite from ~0.9 s to ~5.3 s. Inherent to driving real deadlines against real sockets. </details> <details> <summary><b>Verification</b></summary> - `npm test -w @argent/telemetry` - **336 passed**, 19 files - `typecheck:tests`, `eslint --max-warnings 0`, `prettier --check`, `knip`, `tsc --build` all clean; CI green - **Mutation-tested**, whole-suite counts rather than one test each: | mutation in `src/otel.ts` | tests red | |---|---| | `compression` pin dropped | 2 | | `compression` pinned to `"gzip"` | 5 | | any one of the 8 cleared env-var names dropped | 1 each | | env-clearing loop deleted | 2 | | the `finally` that restores it deleted | 1 | | `timeoutMillis` dropped | 5 | | `httpAgentOptions` dropped, or just its `timeout` | 1 | | `keepAlive` flipped to `false` | 2 | | `installDiagLogger`'s `!isDebugEnabled()` guard removed | 1 | | `maxExportBatchSize` 20 → 19 or 25; `scheduledDelayMillis` changed | 2 | | `SERVICE_NAME` renamed | 2 | | `LOGGER_NAME` changed | 1 | | record body no longer the event name | 4 | | `severityText` INFO → WARN | 1 | | null/undefined property skip removed | 2 | | `url` given a suffix the config did not set | 9 | - **Absence assertions carry positive controls.** The certificate test reads its answer out of a `Failed to read ` prefix that lives in a transitive dependency, and the 204 case asserts an empty diag channel; both would pass just as well against a dead observer. Each now proves its own observer live first. - **Flake-tested.** The 503 case's exact two-request assertion used to turn on the jitter draw: standing a 500 ms collector delay in for a loaded runner made it fail 20/20 (13 on the request count, 7 on `Timeout` masking the error class). With the jitter pinned, 20/20 pass. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved telemetry export reliability by preventing unintended compression and certificate-related environment settings from affecting exporter configuration. - Ensured telemetry attributes with null or undefined values are omitted rather than serialized as misleading empty values. - Improved handling of unreachable or unresponsive telemetry collectors with bounded failures and appropriate diagnostic reporting. - Preserved connection reuse and batching behavior for more efficient OTLP delivery. - **Tests** - Added comprehensive coverage for OTLP formatting, authorization, batching, compression, certificates, connection handling, and failure scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->