feat(node): metrics autocapture for HTTP, database and runtime metrics - #5149
Draft
DanielVisca wants to merge 1 commit into
Draft
DanielVisca wants to merge 1 commit into
DanielVisca wants to merge 1 commit into
Conversation
…trics
`metrics: { autocapture: true }` starts the official OpenTelemetry instrumentations with private meter and tracer providers and exports to `/i/v1/metrics`. `node --import posthog-node/metrics/register` starts it before the app loads any module. The OpenTelemetry packages are optional peer dependencies.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Generated-By: PostHog Desktop
Task-Id: ef9b57c2-47ff-47e6-a509-8873ffe1fab4
Contributor
Contributor
|
[High risk] Adds OpenTelemetry instrumentation and metrics export to the Node SDK. The PR is not ready to merge because a trailing-slash host can misroute exports and signal termination loses the final metrics window. Reviews (1) · Last reviewed commit: "feat(node): add metrics autocapture for ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| new sdkMetrics.PeriodicExportingMetricReader({ | ||
| exporter: gateExporter( | ||
| new exporter.OTLPMetricExporter({ | ||
| url: `${options.host}/i/v1/metrics`, |
Contributor
There was a problem hiding this comment.
POSTHOG_HOST ends in /, the exporter appends /i/v1/metrics and sends autocaptured metrics to a //i/v1/metrics path instead of the intended endpoint. Construct the endpoint URL from the host so its trailing slash cannot change the path.
Suggested change
| url: `${options.host}/i/v1/metrics`, | |
| url: new URL('/i/v1/metrics', options.host).toString(), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/node/src/extensions/metrics-autocapture.node.ts
Line: 290
Comment:
**Trailing slash misroutes metrics** If `POSTHOG_HOST` ends in `/`, the exporter appends `/i/v1/metrics` and sends autocaptured metrics to a `//i/v1/metrics` path instead of the intended endpoint. Construct the endpoint URL from the host so its trailing slash cannot change the path.
```suggestion
url: new URL('/i/v1/metrics', options.host).toString(),
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+36
to
+38
| process.once('beforeExit', () => { | ||
| void posthog.shutdown() | ||
| }) |
Contributor
There was a problem hiding this comment.
node --import setup, SIGTERM does not run beforeExit. Because this is the registration's only shutdown handler, termination skips the final metrics flush and loses the current window. Provide a signal-aware shutdown path or a way for the application to shut down the registration's client.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/node/src/entrypoints/metrics-register.node.ts
Line: 36-38
Comment:
**Signal exit loses final metrics** With the documented `node --import` setup, SIGTERM does not run `beforeExit`. Because this is the registration's only shutdown handler, termination skips the final metrics flush and loses the current window. Provide a signal-aware shutdown path or a way for the application to shut down the registration's client.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+142
to
+149
| const collection = firstString(attributes, [ | ||
| 'db.collection.name', | ||
| 'db.mongodb.collection', | ||
| 'db.sql.table', | ||
| 'db.cassandra.table', | ||
| ]) | ||
| const namespace = firstString(attributes, ['db.namespace', 'db.name']) | ||
| const address = firstString(attributes, ['server.address', 'net.peer.name']) |
Contributor
There was a problem hiding this comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/node/src/extensions/metrics-autocapture.node.ts
Line: 142-149
Comment:
**Database dimensions can multiply series** The histogram retains collection, namespace, and server-address strings as metric attributes without bounding them. In an app where these values vary by tenant or database instance, this can create many time series and increase metrics costs. Limit or omit dimensions whose cardinality cannot be bounded.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+385
to
+398
| config: { http: true, db: true, runtime: true }, | ||
| serviceName: 'probe', | ||
| resourceAttributes: {}, | ||
| distroVersion: '1.2.3', | ||
| logger: logger(), | ||
| isEnabled: () => true, | ||
| }) | ||
|
|
||
| it('starts autocapture with metrics.autocapture and stops it on shutdown', async () => { | ||
| const posthog = new PostHog('phc_test_token', { host: posthogHost.url, metrics: { autocapture: true } }) | ||
| expect(tryStart()).toBeUndefined() | ||
|
|
||
| await posthog.shutdown() | ||
| const probe = tryStart() |
Contributor
There was a problem hiding this comment.
node --import posthog-node/metrics/register entrypoint from a built package. A subprocess test would catch package-export, ESM-loader, and exit-flush regressions that in-process tests cannot observe.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/node/src/__tests__/metrics-autocapture.spec.ts
Line: 385-398
Comment:
**Preload entrypoint lacks coverage** These tests start autocapture from source but never launch the documented `node --import posthog-node/metrics/register` entrypoint from a built package. A subprocess test would catch package-export, ESM-loader, and exit-flush regressions that in-process tests cannot observe.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Node SDK compliance v2Commit: 443c3d9 Capture v0✅ 55 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration:yaml-parity-v1:feature_flags:disable_geoip_omitted_defaults_to_false — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:143 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. Capture v1✅ 120 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration:yaml-parity-v1:feature_flags:disable_geoip_omitted_defaults_to_false — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:143 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. |
Contributor
|
Size Change: +39.4 kB (+0.17%) Total Size: 23.6 MB 📦 View Changed
ℹ️ View Unchanged
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Metrics needs hand-written
posthog.metrics.*calls today. We want metrics to be the easiest setup: install the SDK, set one flag, and get HTTP, database and runtime metrics.Why: posthog-python#967 was closed because a hand-rolled HTTP wrapper duplicates OpenTelemetry instrumentation, and PostHog accepts OTLP directly. So this change starts the official OpenTelemetry instrumentations for you instead of writing new ones.
Changes
metrics: { autocapture: true }(or{ http, db, runtime }toggles). Off by default.node --import posthog-node/metrics/register app.js(readsPOSTHOG_PROJECT_TOKEN,POSTHOG_HOST,OTEL_SERVICE_NAME). It starts before the app loads modules, and registers the ESM loader hook, so ESM apps are measured too.db.client.operation.duration. pg and oracledb record it natively and are not counted twice. Only bounded attributes; never the statement.telemetry.distro.name=posthog-nodeso we can measure adoption.Known gap:
http.server.request.durationhas nohttp.routeyet (that needs a global context manager, which could clash with an app's own OpenTelemetry setup).How I tested it
src/__tests__/metrics-autocapture.spec.ts(16 tests). Fullposthog-nodesuite: 1117 passed.check-types,lint,formatclean.hogliwith themetricsintent): an ESM Express + pg + ioredis app started with--import posthog-node/metrics/register. In ClickHouse I sawhttp.server.request.duration,http.client.request.duration(fetch),db.client.operation.duration(pg native + ioredis derived),db.client.connection.*and 15 runtime metrics (nodejs.eventloop.*,v8js.*). No series for the PostHog host.requireParentSpan: false, the runtime gauges were lost on shutdown). Each now has a test.Release info Sub-libraries affected
Libraries affected
Checklist
metrics.autocapturewarns there)If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Created with PostHog Desktop
🤖 Generated with Claude Code