chore: Improving OpAmp telemetry

.changeset/opamp-telemetry-context.md

@@ -0,0 +1,16 @@
+---
+"@hyperdx/api": patch
+---
+
+Improve API telemetry quality. The OpAMP message handler span is now a wide event
+carrying agent correlation and self-description context (instance UID, sequence
+number and gap, raw + decoded capability flags, new-vs-existing, service name and
+version, OS type, host arch, health/last-error/uptime, remote config apply
+status/error, last-applied and sent config hashes for drift detection, effective
+config presence/size, teams count, request/response sizes). A new
+`hyperdx.opamp.remote_config_applications` counter tracks whether pushed configs
+actually applied on agents. The shared error middleware now recognizes body-parser
+errors: client disconnects (`request.aborted` / `ECONNABORTED`) are classified as
+operational, logged at debug instead of error, and kept out of error tracking, and
+`hyperdx.api.errors` gains a bounded `error_type` dimension so aborts, oversized
+bodies, and malformed payloads are distinguishable.

.husky/pre-commit

@@ -4,3 +4,10 @@ set -e
 . "$(dirname -- "$0")/_/husky.sh"
 
 npx lint-staged
+
+# Whole-project unused-code analysis. Runs the same check as the Knip CI
+# workflow so unused files/deps/exports are caught before push, not in CI.
+# --no-config-hints matches CI (which never counts hints) and drops the
+# cosmetic ".claude/** redundant ignore" false positive.
+echo "Running knip..."
+npx knip --no-config-hints

AGENTS.md

@@ -92,8 +92,12 @@ before stopping.
    unit/integration tests
 7. **Observability**: This is an observability product - instrument new code as
    you write it. Every team-scoped operation must carry team/user context
-   (`setBusinessContext`), countable log events should also emit a metric, and
-   spans/metric attributes must stay low-cardinality. Use the shared helpers in
+   (`setBusinessContext`), and countable log events should also emit a metric.
+   For our own instrumentation we favor wide events — enrich the unit-of-work
+   span with rich, high-cardinality attributes and keep only span _names_ and
+   _metric_ attributes low-cardinality — while metrics stay first-class
+   (counters/histograms feed alerts and SLOs, and many deployments rely on
+   them). Use the shared helpers in
    `packages/api/src/utils/instrumentation.ts`. See
    [`agent_docs/observability.md`](agent_docs/observability.md).
 

agent_docs/observability.md

@@ -14,13 +14,25 @@ aligned with the
    `setBusinessContext(...)` so `hyperdx.team.id` / `user.id` end up on
    the trace. Auth middleware already does this for HTTP requests; background
    jobs and other entry points must do it themselves.
-2. **If something is worth a log, it's usually worth a metric.** When a log line
-   marks a countable event (an error, a skip, a fired alert, a query), emit a
-   counter or histogram alongside it.
-3. **Spans and metric attributes must be low-cardinality.** Never put raw
-   queries, user input, IDs, or error messages into a span _name_ or a metric
-   _attribute key/value enum_. IDs belong in span _attributes_, not names.
-4. **Use the shared helpers** in
+2. **If something is worth a log, put it on the span first.** The active span is
+   the wide event for the current unit of work — attach the fact there as an
+   attribute. If it also marks a countable event worth aggregating (an error, a
+   skip, a fired alert, a query), emit a counter or histogram alongside it.
+3. **Cardinality belongs on span _attributes_, not span _names_ or _metrics_.**
+   Never put raw queries, user input, IDs, or error messages into a span _name_
+   or a metric _attribute key/value_ — those must stay low-cardinality. Span
+   _attributes_ are the opposite: enrich them freely with high-cardinality
+   context (IDs, sizes, statuses), because that is what makes a trace queryable
+   after the fact.
+4. **Favor wide events for our own code — metrics stay first-class.** For the
+   instrumentation _in this repo_ we lean on richly-attributed spans: before
+   adding an instrument, ask whether the value belongs on the span for the work
+   already in flight, and put point-in-time / gauge-style values (sizes, counts,
+   depths, current state) there rather than in a gauge. This is an internal
+   engineering preference, not a rule against metrics — counters and histograms
+   remain first-class, feed alerts and SLOs, and many HyperDX deployments depend
+   heavily on them. See [Wide events over gauges](#wide-events-over-gauges).
+5. **Use the shared helpers** in
    [`packages/api/src/utils/instrumentation.ts`](../packages/api/src/utils/instrumentation.ts).
    Don't hand-roll tracer/meter lifecycle.
 
@@ -103,11 +115,60 @@ Standard attribute keys:
 | `hyperdx.<domain>.<field>` | Domain IDs, e.g. `hyperdx.alert.id`, `hyperdx.source.id`.  |
 | `feature_flag.<name>`      | Evaluated feature/config flag state.                       |
 
+### Wide events over gauges
+
+> **Scope:** this is HyperDX's _internal_ instrumentation philosophy for the
+> code in this repo — not a recommendation that users pick wide events over
+> metrics. Metrics are a first-class HyperDX signal that many operators rely on;
+> nothing here discourages them. What follows is simply how _we_ prefer to
+> instrument our own services.
+
+We instrument in the spirit of
+[wide events](https://boristane.com/blog/observability-wide-events-101/): one
+richly-attributed span per unit of work beats a scatter of pre-aggregated
+metrics. Pre-aggregation only answers the questions you thought to ask in
+advance; a wide event lets you slice by any dimension _after_ the incident —
+"aborted uploads, but only from agents on this collector version, over 5 MB" is
+a trace query, not a metric you had the foresight to define.
+
+The practical consequence for our code: **default away from gauges.** A gauge is
+a point-in-time sample of a value, and that value almost always belongs to some
+unit of work — a request body size, a batch length, a queue depth at dequeue, a
+team count used to build a config. Prefer putting it on that operation's span as
+an attribute. There it keeps its correlations (which agent, which team, which
+outcome) and stays queryable across every other attribute on the event; a gauge
+sheds all of that the moment it's recorded.
+
+```ts
+// Don't: a bare gauge, stripped of the context that makes it useful.
+// meter.createObservableGauge('opamp.request.body_size')...
+
+// Do: attach the point-in-time value to the span for the work in flight, where
+// it can be sliced by agent, team, outcome, or any other span attribute.
+span.setAttribute('opamp.request.body_size_bytes', req.body.length);
+span.setAttribute('opamp.teams.count', teams.length);
+```
+
+Metrics still earn their place for **aggregate signals that must exist
+independently of any single event** — availability/latency SLIs, error rates,
+alert thresholds — because those stay correct under trace sampling and are cheap
+to alert on. That's the counters and histograms below. What we skip is the
+gauge: if a value is worth recording at a point in time, the span is where it
+belongs.
+
+The rare exception is an ambient value with no owning operation (a pool size or
+queue depth sampled by a background poller). Even then, prefer emitting a
+periodic wide event — a heartbeat span carrying the readings — over a raw gauge,
+so the readings stay sliceable; fall back to a gauge only when no such event
+exists.
+
 ### Metrics
 
 When you write a log line that marks a countable event, add a metric too.
-Counters for occurrences, histograms for durations/sizes. Prefer counters and
-histograms over gauges.
+Counters for occurrences, histograms for durations/sizes. For our own code we
+favor span attributes over gauges (see
+[Wide events over gauges](#wide-events-over-gauges)), but metrics themselves are
+first-class — reach for them for aggregate signals, alerts, and SLOs.
 
 ```ts
 import {

knip.json

@@ -36,7 +36,9 @@
   "ignore": [
     "scripts/dev-portal/**",
     ".github/scripts/**",
-    "docker/hyperdx/**"
+    ".github/actions/**",
+    "docker/hyperdx/**",
+    ".claude/**"
   ],
   "ignoreBinaries": ["make", "migrate"],
   "ignoreDependencies": [

packages/api/src/middleware/__tests__/error.test.ts

@@ -0,0 +1,108 @@
+import type { Request, Response } from 'express';
+
+// --- mocks (hoisted; names must be prefixed with `mock`) ---
+const mockCounterAdd = jest.fn();
+const mockRecordException = jest.fn();
+const mockSetAttribute = jest.fn();
+const mockLogger = { warn: jest.fn(), error: jest.fn(), debug: jest.fn() };
+
+jest.mock('@hyperdx/node-opentelemetry', () => ({
+  recordException: (...args: unknown[]) => mockRecordException(...args),
+}));
+jest.mock('@opentelemetry/api', () => ({
+  trace: { getActiveSpan: () => ({ setAttribute: mockSetAttribute }) },
+}));
+jest.mock('@/config', () => ({ IS_PROD: false }));
+jest.mock('@/utils/instrumentation', () => ({
+  getCounter: () => ({ add: (...args: unknown[]) => mockCounterAdd(...args) }),
+}));
+jest.mock('@/utils/logger', () => ({
+  __esModule: true,
+  default: mockLogger,
+}));
+
+import { appErrorHandler } from '@/middleware/error';
+import { Api404Error } from '@/utils/errors';
+
+const invoke = (err: unknown): Response => {
+  const res = {
+    headersSent: false,
+    status: jest.fn().mockReturnThis(),
+    json: jest.fn(),
+  } as unknown as Response;
+  appErrorHandler(err as any, {} as Request, res, jest.fn());
+  return res;
+};
+
+const lastCounterAttrs = () =>
+  mockCounterAdd.mock.calls[mockCounterAdd.mock.calls.length - 1][1];
+
+describe('appErrorHandler', () => {
+  beforeEach(() => jest.clearAllMocks());
+
+  describe('client disconnect (body-parser request.aborted)', () => {
+    const abortErr = Object.assign(new Error('request aborted'), {
+      type: 'request.aborted',
+      statusCode: 400,
+      received: 5,
+      expected: 10,
+    });
+
+    it('classifies as operational and logs at debug, not error', () => {
+      invoke(abortErr);
+      expect(mockLogger.debug).toHaveBeenCalledTimes(1);
+      expect(mockLogger.error).not.toHaveBeenCalled();
+      expect(mockLogger.warn).not.toHaveBeenCalled();
+      expect(lastCounterAttrs()).toMatchObject({
+        operational: true,
+        error_type: 'request.aborted',
+      });
+    });
+
+    it('skips recordException (benign background noise)', () => {
+      invoke(abortErr);
+      expect(mockRecordException).not.toHaveBeenCalled();
+    });
+
+    it('records how far the upload got on the span', () => {
+      invoke(abortErr);
+      expect(mockSetAttribute).toHaveBeenCalledWith(
+        'http.request.received_bytes',
+        5,
+      );
+      expect(mockSetAttribute).toHaveBeenCalledWith(
+        'http.request.expected_bytes',
+        10,
+      );
+    });
+  });
+
+  describe('outbound ECONNABORTED (e.g. axios timeout) is NOT a disconnect', () => {
+    // No body-parser `type` — must not be silently downgraded.
+    const timeoutErr = Object.assign(new Error('timeout of 1000ms exceeded'), {
+      code: 'ECONNABORTED',
+    });
+
+    it('stays a non-operational error: logged at error, recorded', () => {
+      invoke(timeoutErr);
+      expect(mockLogger.error).toHaveBeenCalledTimes(1);
+      expect(mockLogger.debug).not.toHaveBeenCalled();
+      expect(mockRecordException).toHaveBeenCalledTimes(1);
+      expect(lastCounterAttrs()).toMatchObject({ operational: false });
+    });
+  });
+
+  describe('error_type label boundedness', () => {
+    it('uses the bounded class name, not the interpolated BaseError message', () => {
+      invoke(new Api404Error(`Team not found for user ${'abc-123'}`));
+      // Operational (Api4xx) -> warn. The label is the bounded class name —
+      // NOT the interpolated message. BaseError's setPrototypeOf collapses
+      // subclasses to "BaseError"; granularity comes from status_code (404).
+      expect(mockLogger.warn).toHaveBeenCalledTimes(1);
+      expect(lastCounterAttrs()).toMatchObject({
+        error_type: 'BaseError',
+        status_code: 404,
+      });
+    });
+  });
+});

packages/api/src/middleware/error.ts

@@ -1,4 +1,5 @@
 import { recordException } from '@hyperdx/node-opentelemetry';
+import { trace } from '@opentelemetry/api';
 import type { NextFunction, Request, Response } from 'express';
 
 import { IS_PROD } from '@/config';
@@ -8,24 +9,81 @@ import logger from '@/utils/logger';
 
 const apiErrorCounter = getCounter('hyperdx.api.errors', {
   description:
-    'Count of errors handled by the API error middleware, labeled by operational flag and HTTP status code.',
+    'Count of errors handled by the API error middleware, labeled by operational flag, HTTP status code, and error type.',
 });
 
+// raw-body / body-parser attach a bounded string `type` on parse failures
+// (e.g. 'request.aborted', 'entity.too.large', 'entity.parse.failed'). This is
+// a bounded, low-cardinality value safe to use as a metric dimension.
+type BodyParserError = {
+  type?: unknown;
+  received?: unknown;
+  expected?: unknown;
+};
+
+const bodyParserErrorType = (err: unknown): string | undefined => {
+  const type = (err as BodyParserError)?.type;
+  return typeof type === 'string' ? type : undefined;
+};
+
+/**
+ * A client that disconnects mid-request (aborted upload, LB read timeout,
+ * collector restart) is not a server fault. We classify it as operational so
+ * it stays out of the non-operational error signal and off the error log.
+ *
+ * We key strictly on body-parser's `type === 'request.aborted'` — deliberately
+ * NOT on `code === 'ECONNABORTED'`, which is not scoped to the incoming request
+ * (axios uses it for outbound request timeouts) and would otherwise silently
+ * downgrade real server errors from any route.
+ */
+const isClientDisconnect = (err: unknown): boolean =>
+  bodyParserErrorType(err) === 'request.aborted';
+
 // WARNING: need to keep the 4th arg for express to identify it as an error-handling middleware function
 export const appErrorHandler = (
   err: BaseError,
   _: Request,
   res: Response,
   next: NextFunction,
 ) => {
-  const operational = isOperationalError(err);
+  const parserErrorType = bodyParserErrorType(err);
+  const clientDisconnect = isClientDisconnect(err);
+  const operational = clientDisconnect || isOperationalError(err);
   const statusCode = err.statusCode ?? StatusCode.INTERNAL_SERVER;
   apiErrorCounter.add(1, {
     operational,
     status_code: statusCode,
+    // `err.name` on our BaseError subclasses is the constructor's first arg,
+    // which is routinely an interpolated message (e.g. "Team not found for
+    // user <id>") — unbounded. The class name is bounded; note BaseError's
+    // setPrototypeOf collapses its subclasses to "BaseError" (the granular
+    // distinction comes from status_code above).
+    error_type: parserErrorType ?? err.constructor?.name ?? 'unknown',
   });
 
-  if (operational) {
+  // Attach connection-level context to the active span so a single trace shows
+  // why the request failed and, for aborts, how far the upload got.
+  const activeSpan = trace.getActiveSpan();
+  if (activeSpan) {
+    if (parserErrorType) {
+      activeSpan.setAttribute('http.request_error.type', parserErrorType);
+    }
+    const { received, expected } = err as BodyParserError;
+    if (clientDisconnect && typeof received === 'number') {
+      activeSpan.setAttribute('http.request.received_bytes', received);
+      if (typeof expected === 'number') {
+        activeSpan.setAttribute('http.request.expected_bytes', expected);
+      }
+    }
+  }
+
+  if (clientDisconnect) {
+    const { received, expected } = err as BodyParserError;
+    logger.debug(
+      { err, received, expected },
+      'client disconnected mid-request',
+    );
+  } else if (operational) {
     logger.warn({ err }, err.message);
   } else {
     logger.error({ err }, err.message);
@@ -37,12 +95,17 @@ export const appErrorHandler = (
       ? 'Invalid JSON payload'
       : 'Something went wrong :(';
 
-  void recordException(err, {
-    mechanism: {
-      type: 'generic',
-      handled: userFacingErrorMessage ? true : false,
-    },
-  });
+  // A client disconnect is expected background noise, not an exception worth
+  // surfacing in error tracking (the auto-instrumented parser span already
+  // recorded it), so we skip recordException for it.
+  if (!clientDisconnect) {
+    void recordException(err, {
+      mechanism: {
+        type: 'generic',
+        handled: userFacingErrorMessage ? true : false,
+      },
+    });
+  }
 
   if (!res.headersSent) {
     res.status(statusCode).json({