feat(telemetry): backport diagnostics metrics observability to 6.x - #711
juliobguedes wants to merge 7 commits into
Conversation
![arnica-github-connector[bot]](https://avatars.githubusercontent.com/in/144692?s=60&v=4){kind=small}
| forwarded-parse "2.1.2" | ||
| semver "^7.5.2" | ||
|
|
||
| "@opentelemetry/instrumentation-koa@0.47.1": |
There was a problem hiding this comment.
Dependency Risk: @opentelemetry/instrumentation-koa@0.47.1 has 1 transitive vulnerability
🔍 Upgrade to 0.70.0 (minor version) to mitigate all risks
Severity: Low ⬇️
Status: Open 🔴
Suggested reviewers 🧐: @daniyelnnr
More details:
Take action by replying with an [arnica] command 💬
Actions
Use [arnica] or [a] to interact with the Arnica bot to acknowledge or dismiss code risks.
To acknowledge the finding as a valid code risk: [arnica] ack <acknowledge additional details>
To dismiss the risk with a reason: [arnica] dismiss <fp|accept|capacity> <dismissal reason>
Examples
-
[arnica] ack This is a valid code risk and I'm looking into it -
[arnica] dismiss fp Dismissed - Risk Not Accurate: (i.e. False Positive) -
[arnica] dismiss accept Dismiss - Risk Accepted: Allow the risk to exist in the system -
[arnica] dismiss capacity Dismiss - No Capacity: This will need to wait for a future sprint
| version "0.1.0-beta.10" | ||
| resolved "https://registry.yarnpkg.com/@vtex/diagnostics-nodejs/-/diagnostics-nodejs-0.1.0-beta.10.tgz#af255418c0777bf49d02f1e650d654d20f11e513" | ||
| integrity sha512-w5IOo+P1RcGXYZZw5RV4guQFIKpIqmq7reEQRx6qJYDh0RwLFFhr7NS8MNGm792xOs59hyYMulhx8FMmcOXVxA== | ||
| "@vtex/diagnostics-nodejs@0.1.8-io": |
There was a problem hiding this comment.
Dependency Risk: @vtex/diagnostics-nodejs@0.1.8-io has 21 transitive vulnerabilities
🏗️ Recreate the yarn.lock file (run yarn install) to mitigate 71% of all risks
🚀 Upgrade to 1.1.2 (major version) to mitigate 76% of all risks
Severity: Medium
Status: Open 🔴
Suggested reviewers 🧐: @silvadenisaraujo
More details:
Take action by replying with an [arnica] command 💬
Actions
Use [arnica] or [a] to interact with the Arnica bot to acknowledge or dismiss code risks.
To acknowledge the finding as a valid code risk: [arnica] ack <acknowledge additional details>
To dismiss the risk with a reason: [arnica] dismiss <fp|accept|capacity> <dismissal reason>
Examples
-
[arnica] ack This is a valid code risk and I'm looking into it -
[arnica] dismiss fp Dismissed - Risk Not Accurate: (i.e. False Positive) -
[arnica] dismiss accept Dismiss - Risk Accepted: Allow the risk to exist in the system -
[arnica] dismiss capacity Dismiss - No Capacity: This will need to wait for a future sprint
| "@opentelemetry/sdk-trace-base" "1.30.1" | ||
| "@opentelemetry/semantic-conventions" "1.28.0" | ||
|
|
||
| "@opentelemetry/host-metrics@0.35.5": |
There was a problem hiding this comment.
Dependency Risk: @opentelemetry/host-metrics@0.35.5 has 5 transitive vulnerabilities
🔍 Upgrade to 0.39.0 (minor version) to mitigate all risks
Severity: Low ⬇️
Status: Open 🔴
Suggested reviewers 🧐: @daniyelnnr
More details:
Take action by replying with an [arnica] command 💬
Actions
Use [arnica] or [a] to interact with the Arnica bot to acknowledge or dismiss code risks.
To acknowledge the finding as a valid code risk: [arnica] ack <acknowledge additional details>
To dismiss the risk with a reason: [arnica] dismiss <fp|accept|capacity> <dismissal reason>
Examples
-
[arnica] ack This is a valid code risk and I'm looking into it -
[arnica] dismiss fp Dismissed - Risk Not Accurate: (i.e. False Positive) -
[arnica] dismiss accept Dismiss - Risk Accepted: Allow the risk to exist in the system -
[arnica] dismiss capacity Dismiss - No Capacity: This will need to wait for a future sprint
8e4503d to
78c87a5
Compare
…ity-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 <noreply@anthropic.com>
Ports DiagnosticsMetrics and the split traces/metrics/logs telemetry
client from master (7.x) to the 6.x branch, following the
add-observability-to-6x OpenSpec change (TDD/BDD specs, red-green-refactor
tasks):
- Bump @vtex/diagnostics-nodejs to 0.1.8-io, add @vtex/diagnostics-semconv
and the @opentelemetry/{api,host-metrics,instrumentation,instrumentation-koa}
quartet, matching master.
- Rewrite the telemetry client into split traces/metrics/logs clients
(TelemetryClientSingleton), preserving 6.x's dynamic per-request logger
by additionally exposing the raw TelemetryClient via getTelemetryClient().
- Port DiagnosticsMetrics (recordLatency/incrementCounter/setGauge/
runWithBaseAttributes), cluster resource attributes, and Koa/host-metrics
auto-instrumentation verbatim from master.
- Gate everything behind DIAGNOSTICS_TELEMETRY_ENABLED (default off).
- Fix a jest 25 resolver gap (no package.json "exports" map support) that
the new @opentelemetry/otlp-exporter-base subpath import exposed, via
moduleNameMapper in jest.config.js — this was breaking two pre-existing,
unrelated test suites.
- Update ExporterOptions usage in the logger client for a real breaking
change between diagnostics-nodejs versions (path/protocol/headers
removed).
88/88 tests pass; production build compiles clean. One pre-existing,
unrelated test suite (axiosTracing.test.ts, via a TestServer.ts TS
strictness issue) was already failing before this change and is untouched
by it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Follows the same convention as the preceding 6.52.0 backport commit: version bump and changelog entry land together with the feature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes the gap found after deploying 6.53.0-beta.0 to iotest-ju2: the previous commit ported the DiagnosticsMetrics API and telemetry client, but nothing in 6.x's request pipeline ever called them, so no metrics reached ClickHouse despite the feature flag being enabled. - service/index.ts: startApp() now calls initializeTelemetry() and sets global.diagnosticsMetrics before serving requests. - timings.ts: per-request HTTP handler latency + counter, with runWithBaseAttributes-scoped account/route context. - requestStats.ts: request closed/aborted/total counters. - HttpClient/middlewares/metrics.ts: outbound HTTP client metrics (latency, request/cache/retry counters). - HttpAgentSingleton.ts + statusTrack.ts: HTTP agent socket gauges, updated periodically via trackStatus(). - schemaDirectives/Metric.ts: the @Metric GraphQL directive now emits through DiagnosticsMetrics too. All five emission points degrade gracefully (warn + skip) when global.diagnosticsMetrics is unavailable, matching master. Ported master's own tests verbatim where they exist; added a real-class test for the GraphQL directive since master's own test for it never actually imports the real class. 180/180 tests pass (up from 88, since the branch also picked up an unrelated Prometheus-aggregation backport's suites during rebase); same single pre-existing, unrelated axiosTracing.test.ts failure as before. yarn build compiles clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dba1b5e to
13640bd
Compare
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deployed 6.53.0-beta.1 to iotest-ju2 and confirmed via ClickHouse that 6.x apps (and even the pre-existing structured logger, predating this backport) have never delivered a single row of telemetry, in any cluster, ever — despite clean "Telemetry enabled" init logs, correct env vars, open network path, and 7.x apps in the exact same cluster succeeding continuously with the same new metric names. @vtex/diagnostics-nodejs's TelemetryClient supports a debug option that enables the OTel SDK's own diagnostic console logging at DEBUG level, surfacing real export-attempt errors instead of them failing silently. Hardcoded on (not env-gated) since this is a throwaway beta build meant purely to capture that error in pod logs; revert once root-caused. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… exports Root cause of no 6.x telemetry ever reaching ClickHouse, found via the debug:true diagnostic build deployed to iotest-ju2. Pod logs showed: TypeError: Channel credentials must be a ChannelCredentials object yarn install had resolved @grpc/grpc-js into two separate copies: 1.14.4 (from @vtex/diagnostics-nodejs's own ^1.13.4 requirement) and 1.13.3 (from the four @opentelemetry/exporter-*-otlp-grpc packages' ^1.7.1 requirement). The ChannelCredentials object built by one copy failed an instanceof check performed by the other, so every metrics/ traces/logs export failed silently in the background — initialization itself never errored, hence the clean "Telemetry enabled" logs despite zero data ever arriving. master never hit this: its yarn.lock happened to collapse both ranges onto a single 1.13.4 resolution naturally. Added a `resolutions` field pinning @grpc/grpc-js to 1.13.4 (matching master's resolution exactly) so 6.x gets the same single, shared copy. Also reverts the temporary debug:true flag added purely to capture this error, now that root cause is found. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
daniyelnnr
left a comment
daniyelnnr
left a comment
There was a problem hiding this comment.
Nice! Alongside a nit, I left some questions about operational factors of the change
There was a problem hiding this comment.
nit: this component will have tests?
| private static instance: TelemetryClientSingleton; | ||
| private telemetryClient: TelemetryClient | undefined; | ||
| private initializationPromise: Promise<TelemetryClient> | undefined = undefined; | ||
| const APPLICATION_ID = APP.ID || 'vtex-io-app' |
There was a problem hiding this comment.
Not sure if we should reuse this same ID or another one
There was a problem hiding this comment.
In fact, the DK_APP_ID one - this APPLICATION_ID I guess is less important
There was a problem hiding this comment.
These new dependencies work with Node 16? I'm not sure if any of them require Node 20 or higher as the minimal version
2 participants
Summary
Stacked on #710 (the
add-observability-to-6xOpenSpec proposal/design/specs/tasks). Implements those tasks, rewritten for TDD/BDD, against the6.xbranch:@vtex/diagnostics-nodejsto0.1.8-io, adds@vtex/diagnostics-semconvand the@opentelemetry/{api,host-metrics,instrumentation,instrumentation-koa}quartet, matchingmaster.TelemetryClientSingleton), while preserving6.x's dynamic per-request logger by also exposing the rawTelemetryClientviagetTelemetryClient().DiagnosticsMetrics(recordLatency/incrementCounter/setGauge/runWithBaseAttributes), cluster resource attributes, and Koa/host-metrics auto-instrumentation frommaster.DIAGNOSTICS_TELEMETRY_ENABLED(default off).package.json"exports" map support) surfaced by the new@opentelemetry/otlp-exporter-basesubpath import, viamoduleNameMapper— this was breaking two pre-existing, unrelated test suites.Exporters.CreateLogsExporterConfigusage in the logger client for a real breaking change betweendiagnostics-nodejsversions (path/protocol/headersremoved fromExporterOptions).openspec/changes/add-observability-to-6x/in this branch carries the same specs, updated in two places to reflect what implementation revealed (seedesign.md's "Implementation Addenda" andtasks.md).Test plan
yarn jest: 88/88 tests pass across 8 suites (9 new/updated test files, written test-first)yarn build: compiles cleanaxiosTracing.test.ts, via aTestServer.tsTS-strictness issue) is pre-existing and unrelated — unchangedyarn.locktypescriptresolution, zero diff on that fileDIAGNOSTICS_TELEMETRY_ENABLED=truein a non-production workspace (metrics/traces/logs actually reach the OTLP collector) — not done in this session, needs a live workspace6.xversion including this change — left for the team🤖 Generated with Claude Code