From 2d0eddcc4423e603c4c049028a383fb7e9aad90b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BAlio=20Guedes?= Date: Wed, 16 Sep 2026 17:46:12 -0300 Subject: [PATCH 1/2] docs(openspec): propose observability diagnostics for node-vtex-api@6.x Adds an OpenSpec change (proposal, design, TDD/BDD specs, and tasks) for backporting DiagnosticsMetrics and the split traces/metrics/logs telemetry client from master to the 6.x branch, now that the dependency audit shows no Node engine blocker under the node:6.x builder runtime. Co-Authored-By: Claude Sonnet 5 --- .../add-observability-to-6x/.openspec.yaml | 2 + .../changes/add-observability-to-6x/design.md | 54 +++++ .../add-observability-to-6x/proposal.md | 30 +++ .../specs/diagnostics-metrics/spec.md | 201 ++++++++++++++++++ .../changes/add-observability-to-6x/tasks.md | 58 +++++ openspec/config.yaml | 20 ++ 6 files changed, 365 insertions(+) create mode 100644 openspec/changes/add-observability-to-6x/.openspec.yaml create mode 100644 openspec/changes/add-observability-to-6x/design.md create mode 100644 openspec/changes/add-observability-to-6x/proposal.md create mode 100644 openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md create mode 100644 openspec/changes/add-observability-to-6x/tasks.md create mode 100644 openspec/config.yaml diff --git a/openspec/changes/add-observability-to-6x/.openspec.yaml b/openspec/changes/add-observability-to-6x/.openspec.yaml new file mode 100644 index 000000000..f08077847 --- /dev/null +++ b/openspec/changes/add-observability-to-6x/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-16 diff --git a/openspec/changes/add-observability-to-6x/design.md b/openspec/changes/add-observability-to-6x/design.md new file mode 100644 index 000000000..fd85b14e1 --- /dev/null +++ b/openspec/changes/add-observability-to-6x/design.md @@ -0,0 +1,54 @@ +## Context + +`node-vtex-api@6.x` and `master` (`7.x`) have diverged on telemetry: + +- **`6.x` today**: `src/service/telemetry/client.ts` is a `TelemetryClientSingleton` that calls `NewTelemetryClient(...)` once and hands back a single raw `TelemetryClient`. Its only consumer is `src/service/logger/client.ts`, which uses it purely to export structured logs. It depends on `@vtex/diagnostics-nodejs@0.1.0-beta.10`. There is no metrics client, no `DiagnosticsMetrics` API, no semantic-conventions package, no auto-instrumentation. +- **`master` today**: the same singleton pattern was extended into three clients (`newTracesClient`, `newMetricsClient`, `newLogsClient`) built from `@vtex/diagnostics-nodejs@0.1.8-io`, sharing `getClusterResourceAttributes()` for resource attribution, registering `KoaInstrumentation` and `HostMetricsInstrumentation`, and exposing metrics to app/library code through `src/metrics/DiagnosticsMetrics.ts` (histogram-based latency recording, counters, gauges, and OTel-context-scoped "base attributes" merged into every call). Feature activation is gated by `DIAGNOSTICS_TELEMETRY_ENABLED`. + +This was withheld from `6.x` because of a build-time constraint, not a design constraint: `builder-hub` runs `node: 6.x` app builds (including its own — `builder-hub`'s manifest declares `"builders": {"node": "6.x"}`) on the `service-runtime-node@6.x` runtime image, which is `node:16.20.2`. Historically, `yarn install` for `major < 7` builds forked directly from that Node 16 process. When `master`'s diagnostics dependency tree grew, there was no verified path to install it reliably under that process for arbitrary app dependency trees, so 7.x got an escape hatch instead (`service-runtime-node@6.41.0` ships an extra `node20` binary in the same 6.x-branch image; `builder-hub`'s `NodeBuilder`/`yarn.ts` spawns it only when `nodeBuilderMajor >= 7`). + +A dependency audit (done as part of scoping this change) resolved every `package.json` under `@vtex/diagnostics-nodejs@0.1.8-io`, `@vtex/diagnostics-semconv@5.5.2`, and the full `@opentelemetry/*` tree it pulls in (36 packages) plus `@grpc/*`. The highest `engines.node` floor found is `>=14` (`@opentelemetry/api` is `>=8`); nothing requires Node 18+, nothing needs native compilation (`@grpc/grpc-js` ships prebuilt, pure-JS), and nothing is ESM-only. Node 16.20.2 — what `builder-hub` and hosted `6.x` apps already run on — clears every floor in the tree. This removes the original blocker: the port can proceed without touching `builder-hub` or `service-runtime-node`. + +## Goals / Non-Goals + +**Goals:** +- Bring `DiagnosticsMetrics` (latency histogram, counters, gauges, request-scoped base attributes) to `node-vtex-api@6.x`, at parity with `master`'s current public API. +- Bring the split traces/metrics/logs `TelemetryClientSingleton`, cluster resource attributes, and Koa + host-metrics auto-instrumentation to `6.x`. +- Ship the feature dark (`DIAGNOSTICS_TELEMETRY_ENABLED=false` by default) so existing `6.x` apps see no behavior change until explicitly opted in. +- Keep the existing `6.x` structured-logging consumer of the telemetry client working unchanged through the rewrite. + +**Non-Goals:** +- No changes to `builder-hub` or `service-runtime-node` — the dependency audit shows none are required. +- No porting of the `HeaderKeys`/`AttributeKeys` constants refactor from `master`'s `constants.ts` beyond what `AttributeKeys` diagnostics needs (`VTEX_IO_CLUSTER_ID`, `VTEX_IO_CLUSTER_ROLE`, etc.) — that refactor is unrelated cleanup and stays out of scope. +- No attempt to reconcile `6.x` and `master`'s `DiagnosticsMetrics` implementations into a shared package — this is a straight backport, duplication across branches is accepted (consistent with how `node-vtex-api` already maintains divergent major-version branches). +- No new capability surface beyond what `master` already exposes — this change ports existing behavior, it doesn't design new metrics APIs. + +## Decisions + +**Port `master`'s implementation as-is rather than redesigning.** `DiagnosticsMetrics.ts`, `resourceAttributes.ts`, and the split-client shape of `telemetry/client.ts` are stable, tested, and already running in production on `7.x`. Re-deriving them for `6.x` risks behavioral drift between branches for what should be the same feature. The only intentional deviations are the ones required by `6.x`'s existing shape (e.g., `6.x`'s logger already calls `getTelemetryClient()` expecting *a* client back — that call site is adapted to pull `logsClient` from the new multi-client shape rather than rewriting the logger). + +**Bump `@vtex/diagnostics-nodejs` directly from `0.1.0-beta.10` to `0.1.8-io`, not incrementally.** `6.x`'s current usage surface (`NewTelemetryClient`, `TelemetryClient` type) is narrow enough that reviewing the target version's changelog/breaking changes once, at the version already proven on `master`, is lower-risk than bisecting through intermediate betas that were never shipped to a stable consumer. + +**Gate the entire feature behind `DIAGNOSTICS_TELEMETRY_ENABLED`, matching `master`.** Reusing the existing flag (rather than inventing a `6.x`-specific one) keeps operational tooling (dashboards, rollout scripts, on-call runbooks) that already understands this flag from `7.x` valid for `6.x` too. + +**Treat the jest mock for `@vtex/diagnostics-nodejs` as part of this change, not a follow-up.** `6.x` already carries a stub shaped for the old single-client API (added under `test(jest): stub @vtex/diagnostics-nodejs so metrics suites load under jest@25`); it must be updated to the `Exporters`/`Instrumentation`/multi-client shape `master`'s test suite mocks, or the new tests (ported alongside the implementation) won't load. + +## Risks / Trade-offs + +- **[Risk]** `@vtex/diagnostics-nodejs` jumped from a `0.1.0` beta to `0.1.8-io` — an 8-patch, beta-to-"io"-tagged gap whose changelog hasn't been reviewed line-by-line yet. → **Mitigation**: review the package's changelog/tags between the two versions as an explicit task before wiring the new client shape; since `master` already runs `0.1.8-io` in production, any incompatibility surfaces as a diff against known-working behavior, not unknown territory. +- **[Risk]** Every app on the `node: 6.x` builder gains these dependencies transitively (larger `node_modules`, more install time), even for apps that never enable `DIAGNOSTICS_TELEMETRY_ENABLED`. → **Mitigation**: the dependency audit confirms no install-time failure risk (engines, no native builds); the cost is disk/time, not correctness, and is already accepted on `7.x` today. +- **[Risk]** `builder-hub`'s own runtime is Node 16.20.2 today, but that's inferred from the `service-runtime-node` `6.x` branch's `Dockerfile`, not from a live deployment check. If the pinned production image ever diverges from that branch, the "Node 16 clears `>=14`" conclusion still holds unless production somehow regresses below Node 14 — considered very unlikely but not independently verified against the live cluster. → **Mitigation**: no action required given the margin (14 vs. 16), but flagged as an assumption for whoever owns the `builder-hub` deploy pipeline to confirm if they want extra certainty. +- **[Trade-off]** Keeping `6.x` and `master`'s diagnostics code as parallel, duplicated implementations (per Non-Goals) means future diagnostics changes must be ported twice. Accepted because `node-vtex-api` already operates this way across its major-version branches. + +## Migration Plan + +1. Land the dependency bump and telemetry/metrics port on `6.x` with `DIAGNOSTICS_TELEMETRY_ENABLED` defaulting to off (matching `master`'s rollout pattern) — no consumer app sees any behavior change on upgrade. +2. Validate against a small set of `6.x` apps in a non-production workspace with the flag manually enabled, confirming metrics land in the same backend `master`-based apps report to. +3. Roll out the `6.x` `node-vtex-api` version bump to consumer apps at their own pace (standard dependency bump, no forced migration). +4. Enable `DIAGNOSTICS_TELEMETRY_ENABLED` for interested `6.x` apps individually; no fleet-wide flip is required or planned as part of this change. +5. **Rollback**: reverting the `node-vtex-api@6.x` version bump in a consumer app fully reverts behavior — the change is additive and flag-gated, so no data migration or cleanup is needed on rollback. + +## Open Questions + +- Does anyone need `6.x`'s `DiagnosticsMetrics` API to diverge from `master`'s (e.g., different default histogram buckets, different max custom-attribute limit) given `6.x` apps may have different traffic/cardinality profiles, or is exact parity the right target? +- Should the `@vtex/diagnostics-nodejs` changelog review (Risk 1) block this change's merge, or can it happen as a fast-follow given `master` already validates the target version in production? diff --git a/openspec/changes/add-observability-to-6x/proposal.md b/openspec/changes/add-observability-to-6x/proposal.md new file mode 100644 index 000000000..bab345d6f --- /dev/null +++ b/openspec/changes/add-observability-to-6x/proposal.md @@ -0,0 +1,30 @@ +## Why + +`node-vtex-api@6.x` apps currently ship with no metrics observability: the branch only has a bare `TelemetryClient` used exclusively to export structured logs, while `master` (`7.x`) has grown a full traces/metrics/logs stack (`DiagnosticsMetrics`, cluster resource attributes, Koa + host-metrics auto-instrumentation) built on `@vtex/diagnostics-nodejs`. Observability was withheld from `6.x` because `builder-hub` could not reliably build `7.x`-only dependency trees requiring Node > 16 at build time. That build-time isolation gap has since been closed (`service-runtime-node@6.41.0`+ provides a `node20` binary that `builder-hub` spawns for `node: 7.x` app builds), and a dependency audit of the diagnostics/OpenTelemetry package tree (`@vtex/diagnostics-nodejs@0.1.8-io`, `@vtex/diagnostics-semconv@5.5.2`, the `@opentelemetry/*` family) shows its highest `engines.node` floor is `>=14` — well within what the `node: 6.x` builder runtime (Node 16.20.2, per `service-runtime-node`'s `6.x` branch) already supports. There is no longer a technical blocker to bringing metrics observability to `6.x`. + +## What Changes + +- Bump `@vtex/diagnostics-nodejs` in `node-vtex-api@6.x` from `0.1.0-beta.10` to `0.1.8-io`, and add `@vtex/diagnostics-semconv` plus the `@opentelemetry/{api,host-metrics,instrumentation,instrumentation-koa}` dependencies already used on `master`. +- Rewrite `src/service/telemetry/client.ts` from a single bare `TelemetryClient` into the split traces/metrics/logs `TelemetryClientSingleton` shape used on `master`, preserving the existing logger consumer (`src/service/logger/client.ts`) which only needs the logs client. +- Port `src/service/telemetry/resourceAttributes.ts` (cluster id/role resource attributes) and its tests. +- Port `src/metrics/DiagnosticsMetrics.ts` (the `recordLatency` / `incrementCounter` / `setGauge` / `runWithBaseAttributes` public API) and its tests, unchanged from `master`. +- Add the supporting constants already present on `master` but missing on `6.x`: `AttributeKeys` (sourced from `@vtex/diagnostics-semconv`), `CLUSTER_ID`, `CLUSTER_ROLE`, `METRIC_CLIENT_INIT_TIMEOUT_MS`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `DIAGNOSTICS_TELEMETRY_ENABLED`. The `HeaderKeys` refactor on `master` is unrelated cleanup and is **out of scope**. +- Wire Koa auto-instrumentation and host-metrics collection into the `6.x` service bootstrap, gated behind the existing `DIAGNOSTICS_TELEMETRY_ENABLED` env flag so the feature ships dark by default. +- Update jest mocking for `@vtex/diagnostics-nodejs` (`6.x` already stubs an older shape for jest@25 compatibility; the stub needs to match the new client surface). + +No changes to `builder-hub` or `service-runtime-node` are required by this change — the build-time and runtime environments already support this dependency tree on the `node: 6.x` builder. + +## Capabilities + +### New Capabilities +- `diagnostics-metrics`: the `DiagnosticsMetrics` public API (latency histogram, counters, gauges, request-scoped base attributes) and its underlying `TelemetryClientSingleton` (traces/metrics/logs clients, cluster resource attributes, Koa + host-metrics instrumentation) as available to `node-vtex-api@6.x` consumers, feature-flagged via `DIAGNOSTICS_TELEMETRY_ENABLED`. + +### Modified Capabilities +_None — `6.x`'s existing telemetry-backed structured logging keeps its current behavior; it is only extended, not changed._ + +## Impact + +- **Affected code**: `src/service/telemetry/*`, `src/metrics/DiagnosticsMetrics.ts` (new), `src/constants.ts`, `__mocks__/@vtex/diagnostics-nodejs`, jest config for the new mock surface. +- **Dependencies**: `package.json` gains `@vtex/diagnostics-semconv` and the `@opentelemetry/*` quartet; `@vtex/diagnostics-nodejs` is bumped across a major beta-to-stable jump (`0.1.0-beta.10` → `0.1.8-io`) — its own changelog/breaking changes need review during implementation. +- **Consumers**: every VTEX IO app on the `node: 6.x` builder gains these dependencies transitively; verified they resolve under the `6.x` builder's Node 16.20.2 runtime with no native compilation or ESM-resolution requirements. +- **Systems**: no changes needed in `builder-hub` or `service-runtime-node`; this is scoped entirely to `node-vtex-api`. diff --git a/openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md b/openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md new file mode 100644 index 000000000..74cda6fe5 --- /dev/null +++ b/openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md @@ -0,0 +1,201 @@ +## ADDED Requirements + +### Requirement: Latency recording via a shared histogram +`node-vtex-api@6.x` SHALL expose a `DiagnosticsMetrics.recordLatency` method that records a duration, in milliseconds, to a single shared histogram instrument (`io_app_operation_duration_milliseconds`), accepting either a `process.hrtime()` tuple or a raw millisecond number. + +#### Scenario: Recording latency from an hrtime tuple +- **GIVEN** a `DiagnosticsMetrics` instance with an initialized metrics client +- **WHEN** a caller invokes `recordLatency([seconds, nanoseconds], attributes)` +- **THEN** the value is converted to milliseconds (`seconds * 1000 + nanoseconds / 1e6`) +- **AND** the converted value is recorded on the shared latency histogram with the given attributes + +#### Scenario: Recording latency from a millisecond number +- **GIVEN** a `DiagnosticsMetrics` instance with an initialized metrics client +- **WHEN** a caller invokes `recordLatency(42.5, attributes)` +- **THEN** `42.5` is recorded on the shared latency histogram as-is, without unit conversion + +#### Scenario: Recording latency before the metrics client is ready +- **GIVEN** a `DiagnosticsMetrics` instance whose metrics client has not finished initializing +- **WHEN** a caller invokes `recordLatency(...)` +- **THEN** the call is a no-op +- **AND** a warning is logged +- **AND** no exception is thrown + +### Requirement: Named counters and gauges +`node-vtex-api@6.x` SHALL expose `DiagnosticsMetrics.incrementCounter(name, value, attributes)` and `DiagnosticsMetrics.setGauge(name, value, attributes)`, creating and caching one counter or gauge instrument per distinct `name`. + +#### Scenario: Incrementing a counter by name for the first time +- **GIVEN** a `DiagnosticsMetrics` instance with no counter named `http_requests_total` created yet +- **WHEN** a caller invokes `incrementCounter('http_requests_total', 1, attributes)` +- **THEN** a counter instrument named `http_requests_total` is created +- **AND** it is incremented by `1` with the given attributes + +#### Scenario: Reusing an existing counter +- **GIVEN** a counter named `http_requests_total` was already created by a prior `incrementCounter` call +- **WHEN** `incrementCounter('http_requests_total', 1, attributes)` is called again +- **THEN** the previously created counter instrument is reused +- **AND** no duplicate instrument is created for the same name + +#### Scenario: Setting a gauge by name +- **GIVEN** a `DiagnosticsMetrics` instance with no gauge named `cache_items_current` created yet +- **WHEN** a caller invokes `setGauge('cache_items_current', 1024, attributes)` +- **THEN** a gauge instrument named `cache_items_current` is created +- **AND** it is set to `1024` with the given attributes + +#### Scenario: Counter and gauge calls before the metrics client is ready +- **GIVEN** a `DiagnosticsMetrics` instance whose metrics client has not finished initializing +- **WHEN** a caller invokes `incrementCounter(...)` or `setGauge(...)` +- **THEN** the call is a no-op +- **AND** a warning is logged +- **AND** no exception is thrown + +### Requirement: Request-scoped base attributes +`node-vtex-api@6.x` SHALL allow request-scoped "base attributes" to be set via `DiagnosticsMetrics.runWithBaseAttributes(baseAttributes, fn)`, using OpenTelemetry context propagation, so that every metric call made inside `fn` automatically includes those attributes merged with any custom attributes passed to the call. + +#### Scenario: Base attributes are merged into a metric call +- **GIVEN** `runWithBaseAttributes({ 'vtex.account.name': 'mystore' }, fn)` is active +- **WHEN** `fn` calls `recordLatency`, `incrementCounter`, or `setGauge` with its own custom attributes +- **THEN** the recorded attributes include both the base attributes and the call's own custom attributes + +#### Scenario: Base attributes take precedence over conflicting custom attributes +- **GIVEN** `runWithBaseAttributes({ status_code: 200 }, fn)` is active +- **WHEN** `fn` calls a metric method with a custom attribute `{ status_code: 500 }` +- **THEN** the recorded `status_code` attribute is `200` +- **AND** the conflicting custom attribute is silently dropped, without warning or error + +#### Scenario: No base attributes set +- **GIVEN** no `runWithBaseAttributes` scope is active +- **WHEN** a metric call is made with custom attributes +- **THEN** only the call's own custom attributes are recorded + +#### Scenario: Nested base attribute scopes do not leak +- **GIVEN** a metric call is made after a `runWithBaseAttributes` scope has already returned +- **WHEN** that call provides its own custom attributes +- **THEN** the attributes from the completed scope are not applied +- **AND** only the call's own custom attributes are recorded + +### Requirement: Custom attribute cardinality limiting +`node-vtex-api@6.x` SHALL limit the number of custom attributes accepted per metric call to a fixed maximum (7) to control metric cardinality. This limit applies only to custom attributes supplied by callers; base attributes set via `runWithBaseAttributes` are never limited. + +#### Scenario: Custom attributes within the limit +- **GIVEN** a metric call provides 7 or fewer custom attributes +- **WHEN** the call is recorded +- **THEN** all provided custom attributes are recorded unchanged + +#### Scenario: Custom attributes exceeding the limit +- **GIVEN** a metric call provides more than 7 custom attributes +- **WHEN** the call is recorded +- **THEN** only the first 7 (by insertion order) are recorded +- **AND** the remaining attributes are dropped + +#### Scenario: Warning logged for excess attributes in a linked context +- **GIVEN** a metric call provides more than 7 custom attributes +- **AND** the process is running in a linked (production) context +- **WHEN** the call is recorded +- **THEN** a warning is logged naming the number of attributes provided and the limit applied + +#### Scenario: No warning logged for excess attributes outside a linked context +- **GIVEN** a metric call provides more than 7 custom attributes +- **AND** the process is not running in a linked (production) context +- **WHEN** the call is recorded +- **THEN** no warning is logged, and attributes are still truncated to 7 + +#### Scenario: Base attributes are not counted toward the limit +- **GIVEN** base attributes are active in the current scope alongside 7 custom attributes +- **WHEN** the call is recorded +- **THEN** all base attributes are included in full +- **AND** the custom-attribute limit is applied only to the custom attributes + +### Requirement: Split traces, metrics, and logs telemetry clients +`node-vtex-api@6.x` SHALL initialize traces, metrics, and logs clients independently from a single `TelemetryClientSingleton`, backed by `@vtex/diagnostics-nodejs`, replacing the previous single bare `TelemetryClient`. Each client SHALL be created at most once and cached for reuse. + +#### Scenario: Independent client initialization +- **GIVEN** no telemetry clients have been initialized yet +- **WHEN** `getTelemetryClients()` is called +- **THEN** it returns traces, metrics, and logs clients +- **AND** each is initialized from the same underlying `@vtex/diagnostics-nodejs` telemetry client + +#### Scenario: Clients are cached after first initialization +- **GIVEN** `getTelemetryClients()` has already resolved once +- **WHEN** `getTelemetryClients()` is called again +- **THEN** the cached clients are returned +- **AND** `NewTelemetryClient` is not invoked again + +#### Scenario: Concurrent requests during initialization +- **GIVEN** `getTelemetryClients()` initialization is in flight but not yet resolved +- **WHEN** `getTelemetryClients()` is called again before it resolves +- **THEN** all callers receive the result of the same single in-flight initialization +- **AND** no duplicate telemetry clients are created + +#### Scenario: Reset clears cached clients +- **GIVEN** telemetry clients have already been initialized and cached +- **WHEN** `reset()` is called +- **THEN** the next call to `getTelemetryClients()` triggers a fresh initialization + +#### Scenario: Existing structured logging keeps working +- **GIVEN** the structured logger (`src/service/logger/client.ts`) previously depended on a single `getTelemetryClient()` call +- **WHEN** the logger requests a telemetry client after this change +- **THEN** it receives the logs client from the split `TelemetryClientSingleton` +- **AND** it continues exporting logs exactly as before the change + +### Requirement: Cluster resource attributes on emitted telemetry +`node-vtex-api@6.x` SHALL attach cluster identification as resource attributes (`vtex_io.cluster.id`, `vtex_io.cluster.role`) to metrics and logs clients when the corresponding environment values are present, trimming whitespace and omitting empty or missing values. + +#### Scenario: Both cluster values present +- **GIVEN** both `VTEX_CLUSTER_ID` and `VTEX_CLUSTER_ROLE` are set to non-empty values +- **WHEN** the telemetry clients are initialized +- **THEN** both `vtex_io.cluster.id` and `vtex_io.cluster.role` resource attributes are attached to the metrics and logs clients + +#### Scenario: One cluster value missing +- **GIVEN** only `VTEX_CLUSTER_ID` is set (`VTEX_CLUSTER_ROLE` is unset) +- **WHEN** the telemetry clients are initialized +- **THEN** only the `vtex_io.cluster.id` resource attribute is attached +- **AND** `vtex_io.cluster.role` is omitted entirely, not set to an empty string + +#### Scenario: Whitespace-only values are treated as absent +- **GIVEN** `VTEX_CLUSTER_ID` is set to `' '` (whitespace only) +- **WHEN** the telemetry clients are initialized +- **THEN** the `vtex_io.cluster.id` resource attribute is omitted +- **AND** the value is trimmed before the emptiness check is applied + +#### Scenario: Both values absent +- **GIVEN** neither `VTEX_CLUSTER_ID` nor `VTEX_CLUSTER_ROLE` is set +- **WHEN** the telemetry clients are initialized +- **THEN** no cluster resource attributes are attached + +### Requirement: Automatic Koa and host metrics instrumentation +`node-vtex-api@6.x` SHALL register OpenTelemetry auto-instrumentation for Koa request handling and for host-level metrics (event loop, memory, CPU) as part of telemetry client initialization. + +#### Scenario: Koa instrumentation registered on initialization +- **GIVEN** diagnostics telemetry is enabled +- **WHEN** the telemetry clients are initialized +- **THEN** `KoaInstrumentation` is registered against the telemetry client +- **AND** request spans/metrics are captured without any manual instrumentation in app code + +#### Scenario: Host metrics collected automatically +- **GIVEN** diagnostics telemetry is enabled +- **WHEN** the telemetry clients are initialized +- **THEN** `HostMetricsInstrumentation` is started +- **AND** host-level metrics (event loop lag, memory, CPU) are collected automatically without app code calling `DiagnosticsMetrics` directly + +### Requirement: Feature flag gating +`node-vtex-api@6.x` SHALL gate all diagnostics telemetry behavior (client initialization, instrumentation registration, metric emission) behind the `DIAGNOSTICS_TELEMETRY_ENABLED` environment flag, defaulting to disabled. + +#### Scenario: Flag disabled (default) +- **GIVEN** `DIAGNOSTICS_TELEMETRY_ENABLED` is unset +- **WHEN** the service starts +- **THEN** no diagnostics telemetry client is initialized +- **AND** no Koa or host-metrics auto-instrumentation is registered +- **AND** existing `6.x` app behavior is unchanged from before this feature existed + +#### Scenario: Flag explicitly set to a falsy value +- **GIVEN** `DIAGNOSTICS_TELEMETRY_ENABLED` is set to any value other than the literal string `'true'` (e.g. `'false'`, `'0'`, `'no'`) +- **WHEN** the service starts +- **THEN** diagnostics telemetry remains disabled, identically to the unset case + +#### Scenario: Flag enabled +- **GIVEN** `DIAGNOSTICS_TELEMETRY_ENABLED` is set to `'true'` +- **WHEN** the service starts +- **THEN** telemetry clients initialize +- **AND** `DiagnosticsMetrics` becomes usable +- **AND** Koa/host-metrics auto-instrumentation is registered diff --git a/openspec/changes/add-observability-to-6x/tasks.md b/openspec/changes/add-observability-to-6x/tasks.md new file mode 100644 index 000000000..6f4502edb --- /dev/null +++ b/openspec/changes/add-observability-to-6x/tasks.md @@ -0,0 +1,58 @@ +Each numbered group below follows red → green → refactor: write the failing spec-derived tests first, implement the minimum to pass them, then refactor with tests green throughout. No implementation task should be started before its preceding test task is committed and failing for the right reason. + +## 1. Setup (prerequisite for any red test to run) + +- [ ] 1.1 Review the `@vtex/diagnostics-nodejs` changelog/tags between `0.1.0-beta.10` and `0.1.8-io` for breaking changes affecting `NewTelemetryClient`/`TelemetryClient` usage +- [ ] 1.2 Bump `@vtex/diagnostics-nodejs` to `0.1.8-io`, add `@vtex/diagnostics-semconv@5.5.2` and `@opentelemetry/{api,host-metrics,instrumentation,instrumentation-koa}` to `package.json` (versions pinned to `master`), then `yarn install` and confirm no engine warnings/failures on the `6.x` branch +- [ ] 1.3 Update the `@vtex/diagnostics-nodejs` jest mock to expose `Exporters`, `Instrumentation`, `Logs`, `Metrics`, `Traces`, and `NewTelemetryClient`, matching the shape `master`'s test suite mocks — without this, every red test below fails on module resolution rather than on the intended assertion +- [ ] 1.4 Add `AttributeKeys` (from `@vtex/diagnostics-semconv`), `CLUSTER_ID`, `CLUSTER_ROLE`, `METRIC_CLIENT_INIT_TIMEOUT_MS`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `DIAGNOSTICS_TELEMETRY_ENABLED` to `src/constants.ts`, matching `master`'s definitions (the `HeaderKeys` refactor stays out of scope) + +## 2. Cluster resource attributes + +- [ ] 2.1 **Red**: write `src/service/telemetry/resourceAttributes.test.ts` covering the four scenarios under "Cluster resource attributes on emitted telemetry" (both present, one missing, whitespace-only, both absent); confirm it fails because `getClusterResourceAttributes` does not exist yet +- [ ] 2.2 **Green**: implement `src/service/telemetry/resourceAttributes.ts` (`getClusterResourceAttributes`) to make all four scenarios pass +- [ ] 2.3 **Refactor**: clean up trimming/emptiness logic once green; re-run tests after each edit + +## 3. Split telemetry client (traces/metrics/logs) + +- [ ] 3.1 **Red**: write `src/service/telemetry/client.test.ts` covering "Split traces, metrics, and logs telemetry clients" (independent init, caching, concurrent in-flight init, `reset()`) using the updated mock from 1.3; confirm it fails against the current single-client `TelemetryClientSingleton` +- [ ] 3.2 **Red**: extend the same test file (or add a focused test) for "Existing structured logging keeps working" — assert `src/service/logger/client.ts` still exports logs correctly once it depends on the new shape; confirm it fails before the logger is updated +- [ ] 3.3 **Green**: rewrite `src/service/telemetry/client.ts` into the split `TelemetryClientSingleton` (`newTracesClient`/`newMetricsClient`/`newLogsClient`, `getTelemetryClients()`, `reset()`) to pass 3.1 +- [ ] 3.4 **Green**: update `src/service/logger/client.ts` to pull the logs client from `getTelemetryClients()` to pass 3.2 +- [ ] 3.5 **Refactor**: extract shared exporter-config construction if duplicated across the three client initializers; keep 3.1–3.2 green throughout +- [ ] 3.6 Confirm `src/service/telemetry/index.ts` exports the shape both the logger and the new metrics client need + +## 4. Auto-instrumentation (Koa + host metrics) + +- [ ] 4.1 **Red**: extend `src/service/telemetry/client.test.ts` with the two scenarios under "Automatic Koa and host metrics instrumentation" (Koa instrumentation registered, host metrics collection started); confirm it fails before instrumentation is wired in +- [ ] 4.2 **Green**: register `KoaInstrumentation` and `HostMetricsInstrumentation` during telemetry client initialization to pass 4.1 +- [ ] 4.3 **Refactor**: confirm instrumentation registration is skipped cleanly when telemetry is disabled (ties into group 7), without duplicating the enablement check + +## 5. `DiagnosticsMetrics`: latency, counters, gauges + +- [ ] 5.1 **Red**: write `src/metrics/DiagnosticsMetrics.test.ts` covering "Latency recording via a shared histogram" (hrtime conversion, raw milliseconds, not-ready no-op) and "Named counters and gauges" (create-on-first-use, reuse, not-ready no-op); confirm it fails because `DiagnosticsMetrics` does not exist yet +- [ ] 5.2 **Green**: implement `src/metrics/DiagnosticsMetrics.ts` with `recordLatency`, `incrementCounter`, `setGauge`, and the shared histogram/counter-map/gauge-map to pass 5.1 +- [ ] 5.3 **Refactor**: extract `hrtimeToMillis` and the not-ready guard if duplicated across methods + +## 6. `DiagnosticsMetrics`: base attributes and cardinality limiting + +- [ ] 6.1 **Red**: extend `src/metrics/DiagnosticsMetrics.test.ts` with "Request-scoped base attributes" (merge, precedence, no-scope, no-leak-after-scope-ends) and "Custom attribute cardinality limiting" (within limit, exceeding limit, warning in linked context, no warning outside it, base attributes uncounted); confirm it fails before `runWithBaseAttributes`/limiting exist +- [ ] 6.2 **Green**: implement `runWithBaseAttributes` (OTel context propagation), `mergeAttributes`, and `limitCustomAttributes` (limit of 7) to pass 6.1 +- [ ] 6.3 **Refactor**: consolidate the merge/limit pipeline used by all three metric methods + +## 7. Feature flag gating + +- [ ] 7.1 **Red**: write a test (e.g. `src/service/telemetry/featureFlag.test.ts` or an addition to `client.test.ts`) covering "Flag disabled (default)", "Flag explicitly set to a falsy value", and "Flag enabled"; confirm it fails before the gate is wired into client initialization / instrumentation registration +- [ ] 7.2 **Green**: gate telemetry client initialization and instrumentation registration behind `DIAGNOSTICS_TELEMETRY_ENABLED` to pass 7.1 +- [ ] 7.3 **Refactor**: ensure the gate is checked in exactly one place rather than duplicated across traces/metrics/logs/instrumentation setup + +## 8. Full-suite regression and manual verification + +- [ ] 8.1 Run the complete `6.x` jest suite; confirm no regressions in tests outside this change's scope +- [ ] 8.2 Manually verify in a non-production workspace with `DIAGNOSTICS_TELEMETRY_ENABLED=true`: metrics/traces/logs clients initialize, Koa/host-metrics instrumentation registers, and `DiagnosticsMetrics` calls emit data to the configured OTLP endpoint +- [ ] 8.3 Manually verify with the flag unset: no telemetry initialization occurs and app behavior is unchanged from the pre-change baseline + +## 9. Documentation and release + +- [ ] 9.1 Add a `CHANGELOG.md` entry on the `6.x` branch describing the new diagnostics metrics capability and the `DIAGNOSTICS_TELEMETRY_ENABLED` flag +- [ ] 9.2 Release a new `6.x` version of `node-vtex-api` including this change diff --git a/openspec/config.yaml b/openspec/config.yaml new file mode 100644 index 000000000..392946c67 --- /dev/null +++ b/openspec/config.yaml @@ -0,0 +1,20 @@ +schema: spec-driven + +# Project context (optional) +# This is shown to AI when creating artifacts. +# Add your tech stack, conventions, style guides, domain knowledge, etc. +# Example: +# context: | +# Tech stack: TypeScript, React, Node.js +# We use conventional commits +# Domain: e-commerce platform + +# Per-artifact rules (optional) +# Add custom rules for specific artifacts. +# Example: +# rules: +# proposal: +# - Keep proposals under 500 words +# - Always include a "Non-goals" section +# tasks: +# - Break tasks into chunks of max 2 hours From 69f64da19294c4358485adf464f2a3a8b83bf71e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BAlio=20Guedes?= Date: Thu, 17 Sep 2026 14:15:42 -0300 Subject: [PATCH 2/2] docs(openspec): add missing request-pipeline wiring to add-observability-to-6x The first implementation pass (PR #711) ported the DiagnosticsMetrics API and telemetry client, but not the wiring that actually calls them per request. Deploying 6.53.0-beta.0 to the iotest-ju2 test cluster confirmed no metrics reached ClickHouse despite the feature flag being enabled, because: - service/index.ts never calls initializeTelemetry() or sets global.diagnosticsMetrics - none of master's five consumer call sites (timings.ts, requestStats.ts, HttpClient/middlewares/metrics.ts, HttpAgentSingleton.ts, the @metric GraphQL directive) exist on 6.x yet Updates proposal.md, design.md, and specs/diagnostics-metrics/spec.md to cover this wiring (new BDD requirements: boot-time initialization, request-pipeline metric emission, graceful degradation), and rewrites tasks.md as a single source of truth reflecting what's actually done (groups 1-7, 9.1, 9.3, 10.1) versus what's still pending (new group 8: the wiring itself, plus 9.2/9.4/10.2). Co-Authored-By: Claude Sonnet 5 --- .../changes/add-observability-to-6x/design.md | 14 +++- .../add-observability-to-6x/proposal.md | 10 ++- .../specs/diagnostics-metrics/spec.md | 58 ++++++++++++- .../changes/add-observability-to-6x/tasks.md | 84 ++++++++++++------- 4 files changed, 128 insertions(+), 38 deletions(-) diff --git a/openspec/changes/add-observability-to-6x/design.md b/openspec/changes/add-observability-to-6x/design.md index fd85b14e1..eaab1ad06 100644 --- a/openspec/changes/add-observability-to-6x/design.md +++ b/openspec/changes/add-observability-to-6x/design.md @@ -14,6 +14,7 @@ A dependency audit (done as part of scoping this change) resolved every `package **Goals:** - Bring `DiagnosticsMetrics` (latency histogram, counters, gauges, request-scoped base attributes) to `node-vtex-api@6.x`, at parity with `master`'s current public API. - Bring the split traces/metrics/logs `TelemetryClientSingleton`, cluster resource attributes, and Koa + host-metrics auto-instrumentation to `6.x`. +- **Wire `DiagnosticsMetrics` into the actual request pipeline** at boot (`service/index.ts`) and at every existing emission point `master` already has (HTTP handler timings, request-closed/aborted/total counters, outbound HTTP client metrics, HTTP agent metrics, the `@metric` GraphQL directive) — porting the API surface alone produces no observable data; this wiring is what makes metrics actually reach the OTLP endpoint. - Ship the feature dark (`DIAGNOSTICS_TELEMETRY_ENABLED=false` by default) so existing `6.x` apps see no behavior change until explicitly opted in. - Keep the existing `6.x` structured-logging consumer of the telemetry client working unchanged through the rewrite. @@ -33,6 +34,17 @@ A dependency audit (done as part of scoping this change) resolved every `package **Treat the jest mock for `@vtex/diagnostics-nodejs` as part of this change, not a follow-up.** `6.x` already carries a stub shaped for the old single-client API (added under `test(jest): stub @vtex/diagnostics-nodejs so metrics suites load under jest@25`); it must be updated to the `Exporters`/`Instrumentation`/multi-client shape `master`'s test suite mocks, or the new tests (ported alongside the implementation) won't load. +**Wire `global.diagnosticsMetrics` into the request pipeline exactly where `master` does, with the same "guarded, graceful-degradation" pattern.** `master`'s middlewares check `if (global.diagnosticsMetrics) { ... } else { console.warn('DiagnosticsMetrics not available. ... not reported.') }` at every emission point, rather than assuming it's always initialized. Porting that exact guard (rather than assuming initialization always succeeds) means a telemetry-client init failure degrades to "metrics not reported, warning logged" instead of a crash — consistent with how `6.x` already treats telemetry as best-effort for logging. + +## Implementation Addenda + +Decisions made during implementation that this document didn't originally anticipate: + +1. **`TelemetryClientSingleton` exposes a `getTelemetryClient()` getter for the raw `@vtex/diagnostics-nodejs` `TelemetryClient`, in addition to `master`'s three pre-built clients.** `6.x`'s structured logger builds a *dynamic* per-request logs client (`newLogsClient()` with a loggerName derived from `account`/`workspace`/`appName` passed at call time), which `master`'s fixed-at-init three-client shape doesn't support. Exposing the raw client lets the logger keep this exact behavior with zero changes to its call site or public signature — the only diff is what backs `getTelemetryClient()` internally. +2. **jest's bundled resolver (jest 25) doesn't support `package.json` "exports" maps**, so `@opentelemetry/otlp-exporter-base/node-http` — a subpath pulled in transitively by the diagnostics dependency bump — failed to resolve under tests even though Node's own runtime `require()` resolves it fine. Fixed with a `moduleNameMapper` entry in `jest.config.js` pointing straight at the package's build output. This is a test-infrastructure-only fix; it doesn't affect the engines/Node-version analysis above, which is about production runtime resolution, not jest's resolver. +3. **A real breaking change surfaced between `@vtex/diagnostics-nodejs` versions**: `Exporters.CreateLogsExporterConfig`'s `ExporterOptions` type dropped `path`, `protocol`, and `headers` between `0.1.0-beta.10` and `0.1.8-io`. `6.x`'s logger passed all three; they were removed to match the new type (matching `master`'s simpler usage, which only ever passed `endpoint`). +4. **The request-pipeline wiring (this addendum's reason for existing) was missing from the first implementation pass entirely** — found only after deploying to a live test cluster (`iotest-ju2`) and observing no metrics reaching ClickHouse despite the flag being enabled. Root-caused to `service/index.ts` never calling `initializeTelemetry()`/setting `global.diagnosticsMetrics`, and none of the five consumer middlewares/directive existing on `6.x` yet. This is now folded into the Goals/Decisions above and into `tasks.md` rather than treated as a separate follow-up change, since it's required for the capability described in `specs/diagnostics-metrics/spec.md` to do anything observable. + ## Risks / Trade-offs - **[Risk]** `@vtex/diagnostics-nodejs` jumped from a `0.1.0` beta to `0.1.8-io` — an 8-patch, beta-to-"io"-tagged gap whose changelog hasn't been reviewed line-by-line yet. → **Mitigation**: review the package's changelog/tags between the two versions as an explicit task before wiring the new client shape; since `master` already runs `0.1.8-io` in production, any incompatibility surfaces as a diff against known-working behavior, not unknown territory. @@ -42,7 +54,7 @@ A dependency audit (done as part of scoping this change) resolved every `package ## Migration Plan -1. Land the dependency bump and telemetry/metrics port on `6.x` with `DIAGNOSTICS_TELEMETRY_ENABLED` defaulting to off (matching `master`'s rollout pattern) — no consumer app sees any behavior change on upgrade. +1. Land the dependency bump, telemetry/metrics port, **and the request-pipeline wiring** on `6.x` with `DIAGNOSTICS_TELEMETRY_ENABLED` defaulting to off (matching `master`'s rollout pattern) — no consumer app sees any behavior change on upgrade. (Landing the API port without the wiring, as the first implementation pass did, ships something that compiles and tests green but produces no observable data — not a safe intermediate state to call "done.") 2. Validate against a small set of `6.x` apps in a non-production workspace with the flag manually enabled, confirming metrics land in the same backend `master`-based apps report to. 3. Roll out the `6.x` `node-vtex-api` version bump to consumer apps at their own pace (standard dependency bump, no forced migration). 4. Enable `DIAGNOSTICS_TELEMETRY_ENABLED` for interested `6.x` apps individually; no fleet-wide flip is required or planned as part of this change. diff --git a/openspec/changes/add-observability-to-6x/proposal.md b/openspec/changes/add-observability-to-6x/proposal.md index bab345d6f..4b5d41e0a 100644 --- a/openspec/changes/add-observability-to-6x/proposal.md +++ b/openspec/changes/add-observability-to-6x/proposal.md @@ -11,6 +11,14 @@ - Add the supporting constants already present on `master` but missing on `6.x`: `AttributeKeys` (sourced from `@vtex/diagnostics-semconv`), `CLUSTER_ID`, `CLUSTER_ROLE`, `METRIC_CLIENT_INIT_TIMEOUT_MS`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `DIAGNOSTICS_TELEMETRY_ENABLED`. The `HeaderKeys` refactor on `master` is unrelated cleanup and is **out of scope**. - Wire Koa auto-instrumentation and host-metrics collection into the `6.x` service bootstrap, gated behind the existing `DIAGNOSTICS_TELEMETRY_ENABLED` env flag so the feature ships dark by default. - Update jest mocking for `@vtex/diagnostics-nodejs` (`6.x` already stubs an older shape for jest@25 compatibility; the stub needs to match the new client surface). +- **Wire `DiagnosticsMetrics` into the actual request pipeline**, matching `master`: call `initializeTelemetry()` and set `global.diagnosticsMetrics = new DiagnosticsMetrics()` in `src/service/index.ts`'s `startApp()`, and add the corresponding `global.diagnosticsMetrics` emission calls (guarded by a `global.diagnosticsMetrics` existence check, matching `master`'s graceful-degradation pattern) to: + - `src/service/worker/runtime/http/middlewares/timings.ts` (per-request HTTP handler latency + counter, with request-scoped base attributes via `runWithBaseAttributes`) + - `src/service/worker/runtime/http/middlewares/requestStats.ts` (request closed/aborted/total counters) + - `src/HttpClient/middlewares/metrics.ts` (outbound HTTP client metrics) + - `src/HttpClient/middlewares/request/HttpAgentSingleton.ts` (HTTP agent metrics) + - `src/service/worker/runtime/graphql/schema/schemaDirectives/Metric.ts` (`@metric` GraphQL directive) + + Without this wiring, the ported `DiagnosticsMetrics` class and telemetry client are reachable but inert: nothing in `6.x`'s request path ever calls them, so no per-request/per-operation data reaches the configured OTLP endpoint even with the feature flag on. (This gap was found only after deploying `6.53.0-beta.0` to a test cluster and observing no metrics landing in ClickHouse — the original proposal covered porting the API surface but not this wiring layer.) No changes to `builder-hub` or `service-runtime-node` are required by this change — the build-time and runtime environments already support this dependency tree on the `node: 6.x` builder. @@ -24,7 +32,7 @@ _None — `6.x`'s existing telemetry-backed structured logging keeps its current ## Impact -- **Affected code**: `src/service/telemetry/*`, `src/metrics/DiagnosticsMetrics.ts` (new), `src/constants.ts`, `__mocks__/@vtex/diagnostics-nodejs`, jest config for the new mock surface. +- **Affected code**: `src/service/telemetry/*`, `src/metrics/DiagnosticsMetrics.ts` (new), `src/constants.ts`, `__mocks__/@vtex/diagnostics-nodejs`, jest config for the new mock surface, `src/service/index.ts`, and the five request-pipeline middleware/directive files listed above. - **Dependencies**: `package.json` gains `@vtex/diagnostics-semconv` and the `@opentelemetry/*` quartet; `@vtex/diagnostics-nodejs` is bumped across a major beta-to-stable jump (`0.1.0-beta.10` → `0.1.8-io`) — its own changelog/breaking changes need review during implementation. - **Consumers**: every VTEX IO app on the `node: 6.x` builder gains these dependencies transitively; verified they resolve under the `6.x` builder's Node 16.20.2 runtime with no native compilation or ESM-resolution requirements. - **Systems**: no changes needed in `builder-hub` or `service-runtime-node`; this is scoped entirely to `node-vtex-api`. diff --git a/openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md b/openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md index 74cda6fe5..210d32cd0 100644 --- a/openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md +++ b/openspec/changes/add-observability-to-6x/specs/diagnostics-metrics/spec.md @@ -179,23 +179,73 @@ - **AND** host-level metrics (event loop lag, memory, CPU) are collected automatically without app code calling `DiagnosticsMetrics` directly ### Requirement: Feature flag gating -`node-vtex-api@6.x` SHALL gate all diagnostics telemetry behavior (client initialization, instrumentation registration, metric emission) behind the `DIAGNOSTICS_TELEMETRY_ENABLED` environment flag, defaulting to disabled. +`node-vtex-api@6.x` SHALL gate diagnostics telemetry behind the `DIAGNOSTICS_TELEMETRY_ENABLED` environment flag, defaulting to disabled. When disabled, the underlying `@vtex/diagnostics-nodejs` telemetry client is constructed in the SDK's built-in no-op mode (`noop: true`) rather than not constructed at all; auto-instrumentation registration, however, is skipped entirely rather than run in a no-op mode. #### Scenario: Flag disabled (default) - **GIVEN** `DIAGNOSTICS_TELEMETRY_ENABLED` is unset - **WHEN** the service starts -- **THEN** no diagnostics telemetry client is initialized +- **THEN** the telemetry client is initialized with `noop: true`, so no data is actually exported - **AND** no Koa or host-metrics auto-instrumentation is registered - **AND** existing `6.x` app behavior is unchanged from before this feature existed #### Scenario: Flag explicitly set to a falsy value - **GIVEN** `DIAGNOSTICS_TELEMETRY_ENABLED` is set to any value other than the literal string `'true'` (e.g. `'false'`, `'0'`, `'no'`) - **WHEN** the service starts -- **THEN** diagnostics telemetry remains disabled, identically to the unset case +- **THEN** diagnostics telemetry remains in no-op mode, identically to the unset case #### Scenario: Flag enabled - **GIVEN** `DIAGNOSTICS_TELEMETRY_ENABLED` is set to `'true'` - **WHEN** the service starts -- **THEN** telemetry clients initialize +- **THEN** the telemetry client is initialized with `noop: false`, so metrics/traces/logs are actually exported - **AND** `DiagnosticsMetrics` becomes usable - **AND** Koa/host-metrics auto-instrumentation is registered + +### Requirement: Telemetry initialization and `DiagnosticsMetrics` wiring at boot +`node-vtex-api@6.x` SHALL initialize the telemetry clients and make `DiagnosticsMetrics` globally available as part of application startup, not only as a lazy side effect of unrelated code (such as the structured logger) happening to run first. + +#### Scenario: Telemetry initializes at boot +- **GIVEN** the service is starting up +- **WHEN** `startApp()` runs +- **THEN** `initializeTelemetry()` is called before the app begins serving requests +- **AND** `global.diagnosticsMetrics` is set to a `DiagnosticsMetrics` instance, available to all request-handling code + +#### Scenario: Boot does not depend on the logger being used first +- **GIVEN** a request is served before any structured log line has been emitted +- **WHEN** that request completes +- **THEN** `global.diagnosticsMetrics` is already available and usable, because initialization happened at boot rather than being triggered lazily by the logger + +### Requirement: Request-pipeline metric emission +`node-vtex-api@6.x` SHALL emit metrics through `global.diagnosticsMetrics` at the same points `master` does, so that per-request and per-operation data actually reaches the configured OTLP endpoint rather than the ported API surface sitting unused. Each emission point SHALL check for `global.diagnosticsMetrics`'s existence and degrade gracefully (log a warning, skip emission) rather than throw if it is unavailable. + +#### Scenario: HTTP handler latency and counter +- **GIVEN** an inbound HTTP request completes handling +- **WHEN** the `timings` middleware runs +- **THEN** `global.diagnosticsMetrics.recordLatency` is called with the request's total duration +- **AND** `global.diagnosticsMetrics.incrementCounter('http_handler_requests_total', ...)` is called +- **AND** both calls run within a `runWithBaseAttributes` scope carrying the request's account, route id, and route type + +#### Scenario: Request lifecycle counters +- **GIVEN** an HTTP request is closed, aborted, or completed +- **WHEN** the `requestStats` middleware observes that event +- **THEN** the corresponding counter is incremented via `global.diagnosticsMetrics` + +#### Scenario: Outbound HTTP client metrics +- **GIVEN** `node-vtex-api`'s HTTP client makes an outbound request +- **WHEN** the request completes +- **THEN** `global.diagnosticsMetrics` records the corresponding client-side metric, matching `master`'s `HttpClient/middlewares/metrics.ts` behavior + +#### Scenario: HTTP agent metrics +- **GIVEN** the shared HTTP agent handles a socket lifecycle event +- **WHEN** that event occurs +- **THEN** `global.diagnosticsMetrics` records the corresponding agent metric, matching `master`'s `HttpAgentSingleton.ts` behavior + +#### Scenario: GraphQL `@metric` directive +- **GIVEN** a GraphQL field is annotated with the `@metric` directive +- **WHEN** that field resolves +- **THEN** `global.diagnosticsMetrics` records the field's metric, matching `master`'s `Metric.ts` schema directive behavior + +#### Scenario: Graceful degradation when diagnostics metrics are unavailable +- **GIVEN** `global.diagnosticsMetrics` is not set (e.g. telemetry initialization failed) +- **WHEN** any of the above emission points is reached +- **THEN** a warning is logged identifying which metric was not reported +- **AND** request handling continues normally, without throwing diff --git a/openspec/changes/add-observability-to-6x/tasks.md b/openspec/changes/add-observability-to-6x/tasks.md index 6f4502edb..8e8e7c1ae 100644 --- a/openspec/changes/add-observability-to-6x/tasks.md +++ b/openspec/changes/add-observability-to-6x/tasks.md @@ -2,57 +2,77 @@ Each numbered group below follows red → green → refactor: write the failing ## 1. Setup (prerequisite for any red test to run) -- [ ] 1.1 Review the `@vtex/diagnostics-nodejs` changelog/tags between `0.1.0-beta.10` and `0.1.8-io` for breaking changes affecting `NewTelemetryClient`/`TelemetryClient` usage -- [ ] 1.2 Bump `@vtex/diagnostics-nodejs` to `0.1.8-io`, add `@vtex/diagnostics-semconv@5.5.2` and `@opentelemetry/{api,host-metrics,instrumentation,instrumentation-koa}` to `package.json` (versions pinned to `master`), then `yarn install` and confirm no engine warnings/failures on the `6.x` branch -- [ ] 1.3 Update the `@vtex/diagnostics-nodejs` jest mock to expose `Exporters`, `Instrumentation`, `Logs`, `Metrics`, `Traces`, and `NewTelemetryClient`, matching the shape `master`'s test suite mocks — without this, every red test below fails on module resolution rather than on the intended assertion -- [ ] 1.4 Add `AttributeKeys` (from `@vtex/diagnostics-semconv`), `CLUSTER_ID`, `CLUSTER_ROLE`, `METRIC_CLIENT_INIT_TIMEOUT_MS`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `DIAGNOSTICS_TELEMETRY_ENABLED` to `src/constants.ts`, matching `master`'s definitions (the `HeaderKeys` refactor stays out of scope) +- [x] 1.1 Review the `@vtex/diagnostics-nodejs` changelog/tags between `0.1.0-beta.10` and `0.1.8-io` for breaking changes affecting `NewTelemetryClient`/`TelemetryClient` usage — found real breaking changes (extra leading `applicationID` param on `NewTelemetryClient`, `setInstrumentations` renamed to `registerInstrumentations`, config objects became `Partial<...>` overrides); confirmed `master`'s implementation already targets the new shape correctly +- [x] 1.2 Bump `@vtex/diagnostics-nodejs` to `0.1.8-io`, add `@vtex/diagnostics-semconv@5.5.2` and `@opentelemetry/{api,host-metrics,instrumentation,instrumentation-koa}` to `package.json` (versions pinned to `master`), then `yarn install` and confirm no engine warnings/failures on the `6.x` branch — clean install, no warnings +- [x] 1.3 Add a jest mock for `@vtex/diagnostics-semconv` (`__mocks__/@vtex/diagnostics-semconv.ts`, ported from `master`) and mock `@vtex/diagnostics-nodejs` inline per test file (`Exporters`, `Instrumentation`, `Logs`, `Metrics`, `Traces`, `NewTelemetryClient`), matching `master`'s convention — **correction to this task's original wording**: `6.x` had no existing diagnostics-nodejs mock to "update"; that assumption traced back to a commit that only exists on the unrelated `aerie/backport-cluster-wide-prom-client-metrics-aggreg` branch, not `6.x` +- [x] 1.4 Add `AttributeKeys` (from `@vtex/diagnostics-semconv`), `CLUSTER_ID`, `CLUSTER_ROLE`, `METRIC_CLIENT_INIT_TIMEOUT_MS`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `DIAGNOSTICS_TELEMETRY_ENABLED` to `src/constants.ts`, matching `master`'s definitions (the `HeaderKeys` refactor stays out of scope) +- [x] 1.5 (added during implementation) Map `@opentelemetry/otlp-exporter-base/node-http` in `jest.config.js`'s `moduleNameMapper` — jest 25's bundled resolver predates `package.json` "exports" map support, so this OTLP gRPC exporter subpath (newly pulled in transitively by the `diagnostics-nodejs` bump) failed to resolve under tests even though Node itself resolves it fine at runtime; this was breaking two pre-existing, unrelated test suites (`rateLimit.test.ts`, `axiosTracing.test.ts`) ## 2. Cluster resource attributes -- [ ] 2.1 **Red**: write `src/service/telemetry/resourceAttributes.test.ts` covering the four scenarios under "Cluster resource attributes on emitted telemetry" (both present, one missing, whitespace-only, both absent); confirm it fails because `getClusterResourceAttributes` does not exist yet -- [ ] 2.2 **Green**: implement `src/service/telemetry/resourceAttributes.ts` (`getClusterResourceAttributes`) to make all four scenarios pass -- [ ] 2.3 **Refactor**: clean up trimming/emptiness logic once green; re-run tests after each edit +- [x] 2.1 **Red**: write `src/service/telemetry/resourceAttributes.test.ts` covering the four scenarios under "Cluster resource attributes on emitted telemetry" (both present, one missing, whitespace-only, both absent); confirm it fails because `getClusterResourceAttributes` does not exist yet +- [x] 2.2 **Green**: implement `src/service/telemetry/resourceAttributes.ts` (`getClusterResourceAttributes`) to make all four scenarios pass +- [x] 2.3 **Refactor**: clean up trimming/emptiness logic once green; re-run tests after each edit — none needed, ported verbatim from `master` ## 3. Split telemetry client (traces/metrics/logs) -- [ ] 3.1 **Red**: write `src/service/telemetry/client.test.ts` covering "Split traces, metrics, and logs telemetry clients" (independent init, caching, concurrent in-flight init, `reset()`) using the updated mock from 1.3; confirm it fails against the current single-client `TelemetryClientSingleton` -- [ ] 3.2 **Red**: extend the same test file (or add a focused test) for "Existing structured logging keeps working" — assert `src/service/logger/client.ts` still exports logs correctly once it depends on the new shape; confirm it fails before the logger is updated -- [ ] 3.3 **Green**: rewrite `src/service/telemetry/client.ts` into the split `TelemetryClientSingleton` (`newTracesClient`/`newMetricsClient`/`newLogsClient`, `getTelemetryClients()`, `reset()`) to pass 3.1 -- [ ] 3.4 **Green**: update `src/service/logger/client.ts` to pull the logs client from `getTelemetryClients()` to pass 3.2 -- [ ] 3.5 **Refactor**: extract shared exporter-config construction if duplicated across the three client initializers; keep 3.1–3.2 green throughout -- [ ] 3.6 Confirm `src/service/telemetry/index.ts` exports the shape both the logger and the new metrics client need +- [x] 3.1 **Red**: write `src/service/telemetry/client.test.ts` covering "Split traces, metrics, and logs telemetry clients" (independent init, caching, concurrent in-flight init, `reset()`) using the updated mock from 1.3; confirm it fails against the current single-client `TelemetryClientSingleton` +- [x] 3.2 **Red**: extend the same test file for "Existing structured logging keeps working" — added `src/service/logger/client.test.ts` asserting `getLogClient` still builds its per-call (account/workspace/appName) logger correctly +- [x] 3.3 **Green**: rewrite `src/service/telemetry/client.ts` into the split `TelemetryClientSingleton` (`newTracesClient`/`newMetricsClient`/`newLogsClient`, `getTelemetryClients()`, `reset()`) to pass 3.1 — **deviates from `master`**: also exposes `getTelemetryClient()` returning the raw underlying `TelemetryClient` (see design.md addendum) because `6.x`'s logger builds a dynamic per-request logger via `newLogsClient()`, which `master`'s three-pre-built-clients-only surface doesn't support +- [x] 3.4 **Green**: `src/service/logger/client.ts` needed no signature/behavior change — it already imported `getTelemetryClient` by that exact name; only its `Exporters.CreateLogsExporterConfig(...)` call needed updating (see 1.1: `path`/`protocol`/`headers` no longer exist on `ExporterOptions` in `0.1.8-io`) +- [x] 3.5 **Refactor**: not needed — implementation matches `master`'s exporter-config structure per client, no duplication introduced +- [x] 3.6 Confirm `src/service/telemetry/index.ts` exports the shape both the logger and the new metrics client need — already `export * from './client'`, no change needed ## 4. Auto-instrumentation (Koa + host metrics) -- [ ] 4.1 **Red**: extend `src/service/telemetry/client.test.ts` with the two scenarios under "Automatic Koa and host metrics instrumentation" (Koa instrumentation registered, host metrics collection started); confirm it fails before instrumentation is wired in -- [ ] 4.2 **Green**: register `KoaInstrumentation` and `HostMetricsInstrumentation` during telemetry client initialization to pass 4.1 -- [ ] 4.3 **Refactor**: confirm instrumentation registration is skipped cleanly when telemetry is disabled (ties into group 7), without duplicating the enablement check +- [x] 4.1 **Red**: extended `src/service/telemetry/client.test.ts` with instrumentation-registration scenarios (disabled → not registered; enabled → `KoaInstrumentation` registered) +- [x] 4.2 **Green**: ported `src/service/metrics/instruments/hostMetrics.ts` (`HostMetricsInstrumentation`) from `master`; registered alongside `KoaInstrumentation` in `client.ts`, gated by `DIAGNOSTICS_TELEMETRY_ENABLED` +- [x] 4.3 **Refactor**: gate is a single `if (DIAGNOSTICS_TELEMETRY_ENABLED)` block around instrumentation registration, matching `master`; no duplication ## 5. `DiagnosticsMetrics`: latency, counters, gauges -- [ ] 5.1 **Red**: write `src/metrics/DiagnosticsMetrics.test.ts` covering "Latency recording via a shared histogram" (hrtime conversion, raw milliseconds, not-ready no-op) and "Named counters and gauges" (create-on-first-use, reuse, not-ready no-op); confirm it fails because `DiagnosticsMetrics` does not exist yet -- [ ] 5.2 **Green**: implement `src/metrics/DiagnosticsMetrics.ts` with `recordLatency`, `incrementCounter`, `setGauge`, and the shared histogram/counter-map/gauge-map to pass 5.1 -- [ ] 5.3 **Refactor**: extract `hrtimeToMillis` and the not-ready guard if duplicated across methods +- [x] 5.1 **Red**: ported `master`'s `src/metrics/DiagnosticsMetrics.test.ts` verbatim (covers latency/counter/gauge scenarios plus base attributes and limiting from group 6 in one file, exceeding this change's spec coverage); confirmed it fails on missing modules +- [x] 5.2 **Green**: ported `src/metrics/DiagnosticsMetrics.ts` and `src/service/metrics/client.ts` verbatim from `master` +- [x] 5.3 **Refactor**: none needed — verbatim port of already-refactored code ## 6. `DiagnosticsMetrics`: base attributes and cardinality limiting -- [ ] 6.1 **Red**: extend `src/metrics/DiagnosticsMetrics.test.ts` with "Request-scoped base attributes" (merge, precedence, no-scope, no-leak-after-scope-ends) and "Custom attribute cardinality limiting" (within limit, exceeding limit, warning in linked context, no warning outside it, base attributes uncounted); confirm it fails before `runWithBaseAttributes`/limiting exist -- [ ] 6.2 **Green**: implement `runWithBaseAttributes` (OTel context propagation), `mergeAttributes`, and `limitCustomAttributes` (limit of 7) to pass 6.1 -- [ ] 6.3 **Refactor**: consolidate the merge/limit pipeline used by all three metric methods +- [x] 6.1 **Red**: covered by the same ported test file from 5.1 (34 tests total, includes base-attribute merge/precedence/leak and cardinality-limit scenarios) +- [x] 6.2 **Green**: covered by the same verbatim port from 5.2 +- [x] 6.3 **Refactor**: none needed ## 7. Feature flag gating -- [ ] 7.1 **Red**: write a test (e.g. `src/service/telemetry/featureFlag.test.ts` or an addition to `client.test.ts`) covering "Flag disabled (default)", "Flag explicitly set to a falsy value", and "Flag enabled"; confirm it fails before the gate is wired into client initialization / instrumentation registration -- [ ] 7.2 **Green**: gate telemetry client initialization and instrumentation registration behind `DIAGNOSTICS_TELEMETRY_ENABLED` to pass 7.1 -- [ ] 7.3 **Refactor**: ensure the gate is checked in exactly one place rather than duplicated across traces/metrics/logs/instrumentation setup +- [x] 7.1 **Red**: added `src/constants.test.ts` (unset/falsy-values/`'true'` cases) and instrumentation on/off cases already in `client.test.ts` +- [x] 7.2 **Green**: `DIAGNOSTICS_TELEMETRY_ENABLED` gates instrumentation registration in `client.ts`, and is passed as the `noop` option to `NewTelemetryClient` — telemetry clients are always constructed, but run in the SDK's built-in no-op mode when the flag is off; they are not literally skipped. Spec scenario wording updated to match. +- [x] 7.3 **Refactor**: single `noop: !DIAGNOSTICS_TELEMETRY_ENABLED` check plus single instrumentation-registration `if`, matching `master`; no duplication -## 8. Full-suite regression and manual verification +## 8. Request-pipeline wiring (found missing after live-cluster verification) -- [ ] 8.1 Run the complete `6.x` jest suite; confirm no regressions in tests outside this change's scope -- [ ] 8.2 Manually verify in a non-production workspace with `DIAGNOSTICS_TELEMETRY_ENABLED=true`: metrics/traces/logs clients initialize, Koa/host-metrics instrumentation registers, and `DiagnosticsMetrics` calls emit data to the configured OTLP endpoint -- [ ] 8.3 Manually verify with the flag unset: no telemetry initialization occurs and app behavior is unchanged from the pre-change baseline +Deploying `6.53.0-beta.0` to the `iotest-ju2` test cluster with `DIAGNOSTICS_TELEMETRY_ENABLED=true` surfaced that no metrics were reaching ClickHouse. Root cause: groups 1–7 ported the `DiagnosticsMetrics` API and telemetry client, but never wired them into `6.x`'s actual request pipeline — `service/index.ts` never called `initializeTelemetry()`/set `global.diagnosticsMetrics`, and none of `master`'s five consumer call sites exist on `6.x` yet. This group closes that gap. -## 9. Documentation and release +- [ ] 8.1 **Red**: write/extend a test for `src/service/index.ts`'s `startApp()` asserting it calls `initializeTelemetry()` and sets `global.diagnosticsMetrics` to a `DiagnosticsMetrics` instance before serving requests; confirm it fails against the current implementation +- [ ] 8.2 **Green**: update `startApp()` to call `await initializeTelemetry()` and set `global.diagnosticsMetrics = new DiagnosticsMetrics()`, matching `master`; declare the `global.diagnosticsMetrics` type augmentation +- [ ] 8.3 **Red**: extend `src/service/worker/runtime/http/middlewares/timings.ts`'s test coverage with the "HTTP handler latency and counter" scenario (base attributes via `runWithBaseAttributes`, `recordLatency`, `incrementCounter('http_handler_requests_total', ...)`, graceful degradation when `global.diagnosticsMetrics` is unavailable); confirm it fails +- [ ] 8.4 **Green**: port `master`'s `global.diagnosticsMetrics` emission logic into `timings.ts` to pass 8.3 +- [ ] 8.5 **Red**: extend `src/service/worker/runtime/http/middlewares/requestStats.ts`'s test coverage with the "Request lifecycle counters" scenario (closed/aborted/total); confirm it fails +- [ ] 8.6 **Green**: port `master`'s `global.diagnosticsMetrics` emission logic into `requestStats.ts` to pass 8.5 +- [ ] 8.7 **Red**: extend `src/HttpClient/middlewares/metrics.ts`'s test coverage with the "Outbound HTTP client metrics" scenario; confirm it fails +- [ ] 8.8 **Green**: port `master`'s `global.diagnosticsMetrics` emission logic into `HttpClient/middlewares/metrics.ts` to pass 8.7 +- [ ] 8.9 **Red**: extend `src/HttpClient/middlewares/request/HttpAgentSingleton.ts`'s test coverage with the "HTTP agent metrics" scenario; confirm it fails +- [ ] 8.10 **Green**: port `master`'s `global.diagnosticsMetrics` emission logic into `HttpAgentSingleton.ts` to pass 8.9 +- [ ] 8.11 **Red**: extend `src/service/worker/runtime/graphql/schema/schemaDirectives/Metric.ts`'s test coverage with the "GraphQL `@metric` directive" scenario; confirm it fails +- [ ] 8.12 **Green**: port `master`'s `global.diagnosticsMetrics` emission logic into `Metric.ts` to pass 8.11 +- [ ] 8.13 **Refactor**: confirm every emission point uses the same `if (global.diagnosticsMetrics) { ... } else { console.warn(...) }` guard shape as `master`, with no duplicated boilerplate beyond what `master` itself has -- [ ] 9.1 Add a `CHANGELOG.md` entry on the `6.x` branch describing the new diagnostics metrics capability and the `DIAGNOSTICS_TELEMETRY_ENABLED` flag -- [ ] 9.2 Release a new `6.x` version of `node-vtex-api` including this change +## 9. Full-suite regression and manual verification + +- [x] 9.1 Ran the complete `6.x` jest suite after groups 1–7: 88/88 tests pass across 8 suites; 1 pre-existing suite (`axiosTracing.test.ts`) fails on an unrelated TypeScript strictness error in `TestServer.ts` (`resolve()` called with no argument) — confirmed pre-existing via unchanged `yarn.lock` `typescript@4.9.5` resolution and a zero-diff on that file; not caused by this change +- [ ] 9.2 Re-run the full jest suite after group 8 lands; confirm no regressions +- [x] 9.3 Manually verify in a non-production workspace with `DIAGNOSTICS_TELEMETRY_ENABLED=true` — **done, and this is what surfaced the group-8 gap**: deployed `6.53.0-beta.0` to the `iotest-ju2` cluster; telemetry clients initialize (per the flag) but no per-request metrics reached ClickHouse, because nothing called the emission points. Re-verify after group 8 lands that `io_app_operation_duration_milliseconds` and the HTTP/GraphQL counters actually arrive. +- [ ] 9.4 Manually verify with the flag unset in a live workspace: no telemetry initialization side effects (still `noop: true`) and app behavior unchanged from the pre-change baseline + +## 10. Documentation and release + +- [x] 10.1 Added a `CHANGELOG.md` entry (currently under `[6.53.0-beta.0]`) on the `6.x` branch describing the diagnostics metrics capability and the `DIAGNOSTICS_TELEMETRY_ENABLED` flag — update this entry once group 8 lands to mention that metrics are now actually wired into the request pipeline, not just available as a library API +- [ ] 10.2 Release a stable `6.x` version of `node-vtex-api` including this change, once group 8 is verified end-to-end on a test cluster