Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions __mocks__/@vtex/diagnostics-semconv.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@ const ATTR_VTEX_IO_WORKSPACE_NAME = 'vtex_io.workspace.name'
const ATTR_VTEX_IO_WORKSPACE_TYPE = 'vtex_io.workspace.type'
const ATTR_VTEX_IO_APP_ID = 'vtex_io.app.id'
const ATTR_VTEX_IO_APP_AUTHOR_TYPE = 'vtex_io.app.author-type'
const ATTR_VTEX_IO_CLUSTER_ID = 'vtex_io.cluster.id'
const ATTR_VTEX_IO_CLUSTER_ROLE = 'vtex_io.cluster.role'

export {
ATTR_VTEX_ACCOUNT_NAME,
ATTR_VTEX_IO_WORKSPACE_NAME,
ATTR_VTEX_IO_WORKSPACE_TYPE,
ATTR_VTEX_IO_APP_ID,
ATTR_VTEX_IO_APP_AUTHOR_TYPE,
ATTR_VTEX_IO_CLUSTER_ID,
ATTR_VTEX_IO_CLUSTER_ROLE,
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-07-23
61 changes: 61 additions & 0 deletions openspec/changes/add-k8s-cluster-telemetry-dimensions/design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
## Context

The diagnostics telemetry stack is initialized once in `src/service/telemetry/client.ts`. `NewTelemetryClient` receives `additionalAttrs`, which become resource attributes shared by the metrics, logs, and traces clients created from that telemetry client. Today those attributes identify the IO app, vendor, version, and workspace, while signal-specific code adds request and operation dimensions.

Cluster metadata is deployment-level rather than request-level. Adding it independently at every metric and log call site would duplicate logic, miss automatic or future instrumentation, and consume the custom metric attribute budget enforced by `DiagnosticsMetrics`.

## Goals / Non-Goals

**Goals:**

- Export the authoritative Kubernetes cluster identifier/name as `vtex_io.cluster.id`.
- Export the cluster's platform role as `vtex_io.cluster.role`.
- Make both dimensions available on every diagnostics metric and log, including automatically instrumented telemetry.
- Preserve current behavior when either metadata value is unavailable.
- Keep cluster metadata low-cardinality and constant for the process lifetime.

**Non-Goals:**

- Deriving cluster identity from hostnames, pod names, request headers, or network calls.
- Adding pod, node, namespace, or workload dimensions.
- Renaming existing metrics, changing log messages, or modifying legacy console-based telemetry.
- Requiring cluster metadata when running locally or outside Kubernetes.

## Decisions

### Add cluster metadata as telemetry resource attributes

The implementation will extend the `additionalAttrs` passed to `NewTelemetryClient` with non-empty `vtex_io.cluster.id` and `vtex_io.cluster.role` values. This is the single initialization point shared by diagnostics metrics and logs and covers direct instruments, the `DiagnosticsMetrics` wrapper, host metrics, and logger calls.

Per-call instrumentation was rejected because the repository has several independent metric paths and automatic instrumentation. Updating every call site would be error-prone and would count the values against the seven custom attributes accepted by `DiagnosticsMetrics`.

Because the diagnostics clients share one OpenTelemetry resource, traces may also carry these attributes. That consistency is accepted as a side effect; metrics and logs are the required signals.

### Treat runtime metadata as authoritative configuration

The cluster identifier/name will be read from `process.env.VTEX_CLUSTER_ID`, and the cluster role will be read from `process.env.VTEX_CLUSTER_ROLE`. Both values will be normalized by trimming whitespace. Empty, whitespace-only, or undefined values will be omitted independently. `VTEX_REGION` will not be used as a fallback because region and cluster identity have different semantics.

Fallback strings such as `unknown` were rejected because they create an artificial cluster that combines unrelated local or misconfigured workloads.

### Use generated VTEX IO semantic-convention keys

`AttributeKeys.VTEX_IO_CLUSTER_ID` and `AttributeKeys.VTEX_IO_CLUSTER_ROLE` will reference `ATTR_VTEX_IO_CLUSTER_ID` and `ATTR_VTEX_IO_CLUSTER_ROLE` from `@vtex/diagnostics-semconv`, following the existing workspace and app attribute pattern. These generated constants resolve to the stable `vtex_io.cluster.id` and `vtex_io.cluster.role` keys introduced by `vtex/diagnostics#174`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[general.broken-references] 🔵 SUGGEST

The design cites vtex/diagnostics#174 as the source of the vtex_io.cluster.id/vtex_io.cluster.role constants and asserts that 5.5.2 is "the first public version adopted here that contains both cluster constants". Neither claim can be confirmed from this diff, and both are load-bearing: the version choice in package.json rests on them.

Action: Please double-check that vtex/diagnostics#174 is the correct reference and is reachable by readers of this repo, and confirm the 5.5.2 claim against the published changelog.

To dismiss: /dk-review dismiss 5b8ac2f1-3049-4e7a-96d2-8ef07b1a4c65 [reason]

Hard-coded local attribute names were rejected because they would duplicate the semantic-convention package and could drift from the cross-language contract.

## Risks / Trade-offs

- **A runtime does not inject the cluster variables** → Treat both variables as optional and add tests around the resulting constants.
- **Resource attributes also appear on traces** → Accept this because the telemetry client shares a resource and consistent deployment identity is useful across signals.
- **Additional dimensions increase metric series count** → Cluster identity and role are bounded deployment metadata; do not add pod- or request-level values.
- **A deployment omits one value** → Emit the available dimension independently and omit only the missing one.
- **The semantic-convention release is not yet published** → Keep the dependency version unchanged for now and accept temporary build/type-check failures until a release containing `ATTR_VTEX_IO_CLUSTER_ID` and `ATTR_VTEX_IO_CLUSTER_ROLE` is available.
- **A caller emits a data-point attribute with the same key** → Treat `vtex_io.cluster.id` and `vtex_io.cluster.role` as platform resource dimensions and test the diagnostics payload shape at initialization.

## Migration Plan

1. Reference the generated cluster constants from `@vtex/diagnostics-semconv` while retaining the current dependency version until the upstream change is released.
2. Update the dependency to the first published version containing both constants and restore passing build/type-check validation.
3. Deploy without changing existing metric names or log schemas beyond the two optional dimensions.
4. Verify emitted metrics and logs in one development cluster before broad rollout.
5. Roll back by reverting the resource attributes; no stored-data migration is required.
27 changes: 27 additions & 0 deletions openspec/changes/add-k8s-cluster-telemetry-dimensions/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
## Why

Metrics and logs emitted through the diagnostics API cannot currently be grouped or filtered by the Kubernetes cluster that produced them. Adding stable cluster identity and role dimensions will make cross-cluster comparison, incident isolation, and dashboard segmentation possible.

## What Changes

- Read the Kubernetes cluster identifier/name from `VTEX_CLUSTER_ID` and cluster role from `VTEX_CLUSTER_ROLE`.
- Attach the values as the stable `vtex_io.cluster.id` and `vtex_io.cluster.role` semantic-convention dimensions from `@vtex/diagnostics-semconv`.
- Apply the dimensions centrally so built-in and application-provided telemetry receive the same metadata.
- Omit unavailable or empty cluster metadata rather than emitting misleading placeholder values.
- Add coverage for configured and missing cluster metadata.

## Capabilities

### New Capabilities
- `cluster-telemetry-dimensions`: Defines how Kubernetes cluster identity and role are represented on diagnostics metrics and logs.

### Modified Capabilities

None.

## Impact

- Telemetry initialization in `src/service/telemetry/client.ts`.
- Diagnostics metric and log tests, constants for `VTEX_CLUSTER_ID` and `VTEX_CLUSTER_ROLE`, and adoption of the cluster attributes introduced by `vtex/diagnostics#174`.
- Metrics and logs gain two low-cardinality dimensions; dashboards and queries can adopt them without changing metric names or log messages.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[general.broken-references] 🔵 SUGGEST

Second occurrence of the unverifiable vtex/diagnostics#174 reference, in the Impact section.

Action: Verify the reference alongside the one in design.md; if the issue/PR number is wrong, both files need correcting.

To dismiss: /dk-review dismiss c0937e5d-6b21-4d84-8a3f-e1207b6c9a58 [reason]

- The deployment/runtime contract exposes the authoritative Kubernetes cluster identifier/name and role through `VTEX_CLUSTER_ID` and `VTEX_CLUSTER_ROLE`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
## ADDED Requirements

### Requirement: Diagnostics metrics include Kubernetes cluster dimensions
The system SHALL attach configured Kubernetes cluster metadata to every metric emitted through the diagnostics telemetry client. The Kubernetes cluster identifier or name SHALL be exported as `vtex_io.cluster.id`, and the cluster role SHALL be exported as `vtex_io.cluster.role`.

#### Scenario: Both cluster values are configured for metrics
- **WHEN** diagnostics telemetry initializes with non-empty cluster identifier and cluster role metadata
- **THEN** every diagnostics metric has `vtex_io.cluster.id` and `vtex_io.cluster.role` resource dimensions containing the configured values

#### Scenario: Automatically instrumented metric is emitted
- **WHEN** an automatic or built-in diagnostics metric is emitted after telemetry initialization
- **THEN** the metric has the same `vtex_io.cluster.id` and `vtex_io.cluster.role` resource dimensions as an application-provided metric

### Requirement: Diagnostics logs include Kubernetes cluster dimensions
The system SHALL attach configured Kubernetes cluster metadata to every log emitted through the diagnostics telemetry client, using `vtex_io.cluster.id` for the Kubernetes cluster identifier or name and `vtex_io.cluster.role` for the cluster role.

#### Scenario: Both cluster values are configured for logs
- **WHEN** the diagnostics logger emits a log after telemetry initializes with non-empty cluster identifier and cluster role metadata
- **THEN** the log has `vtex_io.cluster.id` and `vtex_io.cluster.role` resource dimensions containing the configured values

#### Scenario: Logs and metrics use consistent cluster values
- **WHEN** a diagnostics metric and diagnostics log are emitted by the same process
- **THEN** both signals have identical `vtex_io.cluster.id` and `vtex_io.cluster.role` values

### Requirement: Missing cluster metadata is omitted safely
The system SHALL normalize cluster metadata by trimming surrounding whitespace and SHALL omit a cluster dimension when its configured value is undefined, empty, or whitespace-only. Availability of one cluster value SHALL NOT depend on availability of the other.

#### Scenario: No cluster metadata is configured
- **WHEN** diagnostics telemetry initializes without a cluster identifier and cluster role
- **THEN** telemetry initialization succeeds and neither `vtex_io.cluster.id` nor `vtex_io.cluster.role` is emitted

#### Scenario: Only cluster identifier is configured
- **WHEN** diagnostics telemetry initializes with a non-empty cluster identifier and no cluster role
- **THEN** emitted diagnostics metrics and logs include `vtex_io.cluster.id` and omit `vtex_io.cluster.role`

#### Scenario: Only cluster role is configured
- **WHEN** diagnostics telemetry initializes with a non-empty cluster role and no cluster identifier
- **THEN** emitted diagnostics metrics and logs include `vtex_io.cluster.role` and omit `vtex_io.cluster.id`

#### Scenario: Cluster metadata contains surrounding whitespace
- **WHEN** configured cluster metadata contains leading or trailing whitespace
- **THEN** emitted cluster dimensions contain the trimmed values

### Requirement: Cluster dimensions use deployment metadata
The system MUST obtain cluster identity from `process.env.VTEX_CLUSTER_ID` and cluster role from `process.env.VTEX_CLUSTER_ROLE` and SHALL keep those values constant for the lifetime of the telemetry client. The system SHALL NOT use `VTEX_REGION` as a fallback for cluster identity. The attribute keys SHALL come from `ATTR_VTEX_IO_CLUSTER_ID` and `ATTR_VTEX_IO_CLUSTER_ROLE` exported by `@vtex/diagnostics-semconv`.

#### Scenario: Telemetry client is initialized
- **WHEN** the process creates its diagnostics telemetry client
- **THEN** it reads cluster identity from `VTEX_CLUSTER_ID` and cluster role from `VTEX_CLUSTER_ROLE` and applies them as resource attributes
- **AND** it uses the generated VTEX IO semantic-convention keys rather than locally defined attribute-name strings

#### Scenario: Request data differs between telemetry calls
- **WHEN** metrics or logs are emitted for different accounts, workspaces, routes, or operations
- **THEN** their cluster dimensions remain the deployment-level values selected at telemetry initialization
21 changes: 21 additions & 0 deletions openspec/changes/add-k8s-cluster-telemetry-dimensions/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
## 1. Runtime Metadata Configuration

- [x] 1.1 Add constants that read cluster identifier/name from `VTEX_CLUSTER_ID` and cluster role from `VTEX_CLUSTER_ROLE`
- [x] 1.2 Expose `ATTR_VTEX_IO_CLUSTER_ID` and `ATTR_VTEX_IO_CLUSTER_ROLE` through `AttributeKeys`, following the existing semantic-convention pattern

## 2. Cluster Metadata Configuration

- [x] 2.1 Add a helper that maps the runtime cluster variables to `vtex_io.cluster.id` and `vtex_io.cluster.role` without using `VTEX_REGION` as a fallback
- [x] 2.2 Add normalization that trims configured values and omits undefined, empty, or whitespace-only metadata independently
- [x] 2.3 Add unit tests covering both values, either value alone, missing values, and whitespace normalization

## 3. Diagnostics Telemetry Integration

- [x] 3.1 Add the normalized semantic-convention cluster values conditionally to `NewTelemetryClient` resource attributes
- [x] 3.2 Add telemetry initialization tests proving diagnostics metrics and logs share `vtex_io.cluster.id` and `vtex_io.cluster.role` without changing metric-call custom attributes
- [x] 3.3 Verify initialization remains successful when neither cluster value is available

## 4. Validation

- [x] 4.1 Confirm current build and focused-test failures are limited to the unpublished `ATTR_VTEX_IO_CLUSTER_ID` and `ATTR_VTEX_IO_CLUSTER_ROLE` exports
- [ ] 4.2 After the upstream semantic-convention release, update `@vtex/diagnostics-semconv` and run repository linting, type checking, and the broader test suite
8 changes: 8 additions & 0 deletions src/constants.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import {
MAX_WORKERS,
LINKED,
REGION,
CLUSTER_ID,
CLUSTER_ROLE,
PUBLIC_ENDPOINT,
APP,
NODE_ENV,
Expand Down Expand Up @@ -130,11 +132,15 @@ describe('constants', () => {
expect(AttributeKeys).toHaveProperty('VTEX_IO_WORKSPACE_TYPE')
expect(AttributeKeys).toHaveProperty('VTEX_IO_APP_ID')
expect(AttributeKeys).toHaveProperty('VTEX_IO_APP_AUTHOR_TYPE')
expect(AttributeKeys).toHaveProperty('VTEX_IO_CLUSTER_ID')
expect(AttributeKeys).toHaveProperty('VTEX_IO_CLUSTER_ROLE')

expect(typeof AttributeKeys.VTEX_IO_WORKSPACE_NAME).toBe('string')
expect(typeof AttributeKeys.VTEX_IO_WORKSPACE_TYPE).toBe('string')
expect(typeof AttributeKeys.VTEX_IO_APP_ID).toBe('string')
expect(typeof AttributeKeys.VTEX_IO_APP_AUTHOR_TYPE).toBe('string')
expect(AttributeKeys.VTEX_IO_CLUSTER_ID).toBe('vtex_io.cluster.id')
expect(AttributeKeys.VTEX_IO_CLUSTER_ROLE).toBe('vtex_io.cluster.role')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Check] 🟡 RESTRICT

The new assertions expect(AttributeKeys.VTEX_IO_CLUSTER_ID).toBe('vtex_io.cluster.id') validate the manual mock, not the real package. __mocks__/@vtex/diagnostics-semconv.ts sits at the project root and mocks a node_modules module, so Jest applies it automatically to every test file without any jest.mock() call (jest config in package.json sets no roots/modulePaths that would exclude it). These assertions therefore pass by construction from the two lines added to the mock and prove nothing about @vtex/diagnostics-semconv@5.5.2. This is the exact claim tasks.md 4.1/4.2 says was verified, so the suite gives false confidence: if 5.5.2 exported a different key string (e.g. vtex_io.k8s.cluster.id), tests stay green and production emits the wrong dimension name.

Action: Add one assertion that imports the real package (bypassing the manual mock via jest.unmock('@vtex/diagnostics-semconv') or jest.requireActual) and checks that ATTR_VTEX_IO_CLUSTER_ID/ATTR_VTEX_IO_CLUSTER_ROLE exist and equal the expected keys — otherwise the mock and the dependency can drift undetected.

To dismiss: /dk-review dismiss 3a1e8f47-2c60-4b19-8d73-f0c9a5e21b8d [reason]

})

test('should have non-empty string values', () => {
Expand Down Expand Up @@ -218,6 +224,8 @@ describe('constants', () => {

test('string environment constants should match their env vars', () => {
expect(REGION).toBe(process.env.VTEX_REGION as string)
expect(CLUSTER_ID).toBe(process.env.VTEX_CLUSTER_ID as string)
expect(CLUSTER_ROLE).toBe(process.env.VTEX_CLUSTER_ROLE as string)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Check] 🔵 SUGGEST

expect(CLUSTER_ID).toBe(process.env.VTEX_CLUSTER_ID as string) is vacuous in CI: VTEX_CLUSTER_ID and VTEX_CLUSTER_ROLE are almost certainly unset there, so both sides evaluate to undefined and the assertion passes regardless of what the constant does. This is the only test covering the env-var → constant link (client.test.ts mocks ../../constants, and resourceAttributes.test.ts passes literals), so nothing in the suite actually proves the code reads VTEX_CLUSTER_ID rather than, say, VTEX_CLUSTER.

Action: Set the env vars to known values and re-require the module (jest.resetModules() + require('./constants')) so the assertion compares a real configured value, or assert against an explicit literal.

To dismiss: /dk-review dismiss e4d71b90-5a2c-4f63-9871-c3b0e5a2df16 [reason]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[quality.new-logic-enforcement] 🔵 SUGGEST

expect(CLUSTER_ID).toBe(process.env.VTEX_CLUSTER_ID as string) compares the constant against the very expression that produced it, so it passes trivially when VTEX_CLUSTER_ID is unset (both sides are undefined) — which is the normal state in CI. The two added lines raise the line-coverage number for the new constants without verifying any behaviour.

Action: Set the env vars to known values before importing the module (or via jest.resetModules() + process.env.VTEX_CLUSTER_ID = 'cluster-a') and assert the concrete value, so the test fails if the constant is wired to the wrong env var name.

To dismiss: /dk-review dismiss 7b3f0c96-4a28-4d51-9e7c-08f6b1d3a542 [reason]

expect(NODE_ENV).toBe(process.env.NODE_ENV as string)
expect(ACCOUNT).toBe(process.env.VTEX_ACCOUNT as string)
expect(WORKSPACE).toBe(process.env.VTEX_WORKSPACE as string)
Expand Down
8 changes: 7 additions & 1 deletion src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import {
ATTR_VTEX_IO_WORKSPACE_NAME,
ATTR_VTEX_IO_WORKSPACE_TYPE,
ATTR_VTEX_IO_APP_ID,
ATTR_VTEX_IO_APP_AUTHOR_TYPE
ATTR_VTEX_IO_APP_AUTHOR_TYPE,
ATTR_VTEX_IO_CLUSTER_ID,
ATTR_VTEX_IO_CLUSTER_ROLE,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Support] 🟡 RESTRICT

The diff imports the new symbols ATTR_VTEX_IO_CLUSTER_ID and ATTR_VTEX_IO_CLUSTER_ROLE from @vtex/diagnostics-semconv, but package.json still declares "@vtex/diagnostics-semconv": "^1.1.2" and is not bumped in this PR. If these attributes were introduced after 1.1.2, any consumer (or a fresh install without the updated lockfile) resolving 1.1.2 gets undefined for both keys, producing resource attributes keyed by undefined instead of a compile/install failure.

Action: Raise the minimum version of @vtex/diagnostics-semconv in package.json to the release that first exports ATTR_VTEX_IO_CLUSTER_ID/ATTR_VTEX_IO_CLUSTER_ROLE (and update yarn.lock in the same PR). If 1.1.2 already exports them, no change is needed — please confirm explicitly.

To dismiss: /dk-review dismiss 5c7e1a80-9f42-4b36-a0d8-72e4c6913bfa [reason]

} from '@vtex/diagnostics-semconv'

// tslint:disable-next-line
Expand Down Expand Up @@ -62,6 +64,8 @@ export const AttributeKeys = {
VTEX_IO_WORKSPACE_TYPE: ATTR_VTEX_IO_WORKSPACE_TYPE,
VTEX_IO_APP_ID: ATTR_VTEX_IO_APP_ID,
VTEX_IO_APP_AUTHOR_TYPE: ATTR_VTEX_IO_APP_AUTHOR_TYPE,
VTEX_IO_CLUSTER_ID: ATTR_VTEX_IO_CLUSTER_ID,
VTEX_IO_CLUSTER_ROLE: ATTR_VTEX_IO_CLUSTER_ROLE,
}

/** @deprecated Use HeaderKeys.CACHE_CONTROL instead */
Expand Down Expand Up @@ -165,6 +169,8 @@ export const MAX_WORKERS = 4

export const LINKED = !!process.env.VTEX_APP_LINK
export const REGION = process.env.VTEX_REGION as string
export const CLUSTER_ID = process.env.VTEX_CLUSTER_ID as string
export const CLUSTER_ROLE = process.env.VTEX_CLUSTER_ROLE as string

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Evolvability.SupportedByLanguage] 🔵 SUGGEST

CLUSTER_ID and CLUSTER_ROLE are cast with as string, but the design and spec explicitly define both as optional deployment metadata that is frequently absent (local runs, non-Kubernetes runtimes). The cast asserts a guarantee the runtime does not provide, so any future consumer that does CLUSTER_ID.toUpperCase() or passes it to a string-typed parameter compiles cleanly and throws or emits "undefined" at runtime. getClusterResourceAttributes happens to accept string | undefined, so the current call site is safe only by coincidence.

Action: Type these as process.env.VTEX_CLUSTER_ID (inferred string | undefined) rather than as string, so the compiler enforces the optionality the spec requires. The pre-existing REGION cast follows the same anti-pattern but is out of scope here.

To dismiss: /dk-review dismiss 9c52d0b6-7e83-4a41-b2f5-1d84c6039e2a [reason]

export const PUBLIC_ENDPOINT = process.env.VTEX_PUBLIC_ENDPOINT || 'myvtex.com'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Evolvability.SupportedByLanguage] 🔵 SUGGEST

CLUSTER_ID/CLUSTER_ROLE are declared as process.env.VTEX_CLUSTER_ID as string, asserting a non-optional string while the values are genuinely optional at runtime (the new getClusterResourceAttributes(clusterId?: string, clusterRole?: string) signature and its 'metadata unavailable' test both acknowledge this). The cast hides the undefined case from every other consumer of these exports.

Action: Type them honestly as export const CLUSTER_ID: string | undefined = process.env.VTEX_CLUSTER_ID (same for CLUSTER_ROLE), so the compiler forces callers to handle absence rather than relying on getClusterResourceAttributes being the only reader.

To dismiss: /dk-review dismiss e4906b23-8d15-4c7a-b2f6-3a81d0e5947c [reason]

export const APP = {
ID: process.env.VTEX_APP_ID as string,
Expand Down
118 changes: 118 additions & 0 deletions src/service/telemetry/client.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
const mockNewTelemetryClient = jest.fn()
const mockCreateExporter = jest.fn()
const mockCreateTracesExporterConfig = jest.fn()
const mockCreateMetricsExporterConfig = jest.fn()
const mockCreateLogsExporterConfig = jest.fn()
const mockGetClusterResourceAttributes = jest.fn()

jest.mock('@vtex/diagnostics-nodejs', () => ({
Exporters: {
CreateExporter: mockCreateExporter,
CreateLogsExporterConfig: mockCreateLogsExporterConfig,
CreateMetricsExporterConfig: mockCreateMetricsExporterConfig,
CreateTracesExporterConfig: mockCreateTracesExporterConfig,
},
Instrumentation: {
CommonInstrumentations: {
minimal: jest.fn(() => []),
},
},
NewTelemetryClient: mockNewTelemetryClient,
}))

jest.mock('../../constants', () => ({
APP: {
ID: 'vtex.test-app@1.0.0',
VENDOR: 'vtex',
VERSION: '1.0.0',
},
AttributeKeys: {
VTEX_IO_APP_ID: 'vtex_io.app.id',
VTEX_IO_WORKSPACE_NAME: 'vtex_io.workspace.name',
VTEX_IO_WORKSPACE_TYPE: 'vtex_io.workspace.type',
VTEX_IO_CLUSTER_ID: 'vtex_io.cluster.id',
VTEX_IO_CLUSTER_ROLE: 'vtex_io.cluster.role',
},
CLUSTER_ID: 'cluster-a',
CLUSTER_ROLE: 'store',
DIAGNOSTICS_TELEMETRY_ENABLED: false,
DK_APP_ID: 'apps-team',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[quality.new-logic-enforcement] 🔵 SUGGEST

The new test file is the only coverage for client.ts, but it hardcodes DIAGNOSTICS_TELEMETRY_ENABLED: false inside the jest.mock factory, so the enabled branch (instrumentation registration, metricsClient.provider(), KoaInstrumentation, HostMetricsInstrumentation) and the catch path can never be exercised. Coverage for initializeTelemetryClients is therefore structurally capped at the disabled path.

Action: Use jest.doMock/jest.isolateModules (or read the flag through a getter) so a third case can set DIAGNOSTICS_TELEMETRY_ENABLED: true and assert registerInstrumentations is called with the host-metrics instrumentation, plus a case where NewTelemetryClient rejects and the error is rethrown.

To dismiss: /dk-review dismiss a2d738f5-6014-4e9b-8c3d-95b7e2f10a68 [reason]

OTEL_EXPORTER_OTLP_ENDPOINT: 'http://collector',
PRODUCTION: true,
WORKSPACE: 'master',
}))

jest.mock('../metrics/instruments/hostMetrics', () => ({
HostMetricsInstrumentation: jest.fn(),
}))

jest.mock('./resourceAttributes', () => ({
getClusterResourceAttributes: mockGetClusterResourceAttributes,
}))

import { initializeTelemetry, resetTelemetry } from './client'

describe('diagnostics telemetry resource attributes', () => {
const tracesClient = {}
const metricsClient = { provider: jest.fn() }
const logsClient = {}
const telemetryClient = {
newLogsClient: jest.fn(),
newMetricsClient: jest.fn(),
newTracesClient: jest.fn(),
registerInstrumentations: jest.fn(),
}

beforeEach(() => {
jest.clearAllMocks()
resetTelemetry()

mockCreateExporter.mockImplementation(config => config)
mockCreateTracesExporterConfig.mockReturnValue({ signal: 'traces' })
mockCreateMetricsExporterConfig.mockReturnValue({ signal: 'metrics' })
mockCreateLogsExporterConfig.mockReturnValue({ signal: 'logs' })
mockNewTelemetryClient.mockResolvedValue(telemetryClient)
telemetryClient.newTracesClient.mockResolvedValue(tracesClient)
telemetryClient.newMetricsClient.mockResolvedValue(metricsClient)
telemetryClient.newLogsClient.mockResolvedValue(logsClient)
})

it('shares configured cluster resource attributes across metrics and logs', async () => {
mockGetClusterResourceAttributes.mockReturnValue({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Evolvability.Textual] 🔵 SUGGEST

The test is named 'shares configured cluster resource attributes across metrics and logs', but its assertions verify the opposite shape: that newMetricsClient and newLogsClient are called WITHOUT additionalAttrs. The actual invariant being tested is that cluster attributes are set once at the NewTelemetryClient resource level and deliberately not repeated per-signal; the name misleads a future reader into thinking per-client attributes are expected.

Action: Rename to something like 'sets cluster resource attributes once on the telemetry resource, not per signal client', so the assertion block matches the stated intent.

To dismiss: /dk-review dismiss c60a95e7-31b8-4f2d-8a74-b9e0c47d1f83 [reason]

'vtex_io.cluster.id': 'cluster-a',
'vtex_io.cluster.role': 'store',
})

const clients = await initializeTelemetry()

expect(mockGetClusterResourceAttributes).toHaveBeenCalledWith('cluster-a', 'store')
expect(mockNewTelemetryClient).toHaveBeenCalledWith(
'apps-team',
'node-vtex-api',
'vtex.test-app@1.0.0',
expect.objectContaining({
additionalAttrs: expect.objectContaining({
'vtex_io.cluster.id': 'cluster-a',
'vtex_io.cluster.role': 'store',
}),
})
)
expect(telemetryClient.newMetricsClient).toHaveBeenCalledTimes(1)
expect(telemetryClient.newLogsClient).toHaveBeenCalledTimes(1)
expect(telemetryClient.newMetricsClient.mock.calls[0][0]).not.toHaveProperty('additionalAttrs')
expect(telemetryClient.newLogsClient.mock.calls[0][0]).not.toHaveProperty('additionalAttrs')
expect(clients).toEqual({ tracesClient, metricsClient, logsClient })
})

it('initializes without cluster dimensions when metadata is unavailable', async () => {
mockGetClusterResourceAttributes.mockReturnValue({})

await initializeTelemetry()

const options = mockNewTelemetryClient.mock.calls[0][3]
expect(options.additionalAttrs).not.toHaveProperty('vtex_io.cluster.id')
expect(options.additionalAttrs).not.toHaveProperty('vtex_io.cluster.role')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Logic] 🟡 RESTRICT

Vacuous assertion: expect(options.additionalAttrs).not.toHaveProperty('vtex_io.cluster.id') — Jest treats a dotted string as a nested property path (options.additionalAttrs.vtex_io.cluster.id), not as the flat key 'vtex_io.cluster.id'. Since the attribute is stored as a single flat key containing dots, this negative assertion can never fail and provides zero regression protection for the 'no cluster metadata' path.

Action: Pass the key as an array to force literal-key matching: expect(options.additionalAttrs).not.toHaveProperty(['vtex_io.cluster.id']), or assert on keys directly, e.g. expect(Object.keys(options.additionalAttrs)).not.toContain('vtex_io.cluster.id').

To dismiss: /dk-review dismiss 3f2a9c14-7b6e-4d21-9a58-1c0e5b8f4d72 [reason]

expect(telemetryClient.newMetricsClient).toHaveBeenCalledTimes(1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Functional.Logic] 🟡 RESTRICT

Second occurrence of the same vacuous assertion: expect(options.additionalAttrs).not.toHaveProperty('vtex_io.cluster.role') is interpreted as the nested path additionalAttrs.vtex_io.cluster.role and therefore always passes, even if the flat key 'vtex_io.cluster.role' is present with a value.

Action: Use the array form not.toHaveProperty(['vtex_io.cluster.role']), and consider asserting the whole object with toEqual so unexpected keys are caught: expect(options.additionalAttrs).toEqual({ 'vtex_io.app.id': ..., vendor: ..., version: ..., 'vtex_io.workspace.name': ..., 'vtex_io.workspace.type': ... }).

To dismiss: /dk-review dismiss b81d4e6f-2c53-4a7e-8f19-6d3b0a97c5e4 [reason]

expect(telemetryClient.newLogsClient).toHaveBeenCalledTimes(1)
})
})
Loading