Harden federation metrics review fixes

dahlia · dahlia · commit 031172db0a74 · 2026-05-05T22:03:03.000+09:00
Make the context meter provider source-compatible for external Context implementations, count all 401 signature rejection paths, and avoid adding full activity payloads to successful delivery span events. Guard queued failure span attributes so malformed inbox payloads do not break retry logic. #755 (comment) #755 (comment) #755 (comment) #755 (comment) #755 (comment) Assisted-by: gpt-5.5
diff --git a/docs/manual/opentelemetry.md b/docs/manual/opentelemetry.md
@@ -289,13 +289,13 @@ Instrumented metrics
 
 Fedify records the following OpenTelemetry metrics:
 
-| Metric name                                  | Instrument | Unit        | Description                                                   |
-| -------------------------------------------- | ---------- | ----------- | ------------------------------------------------------------- |
-| `activitypub.delivery.sent`                  | Counter    | `{attempt}` | Counts outgoing ActivityPub delivery attempts.                |
-| `activitypub.delivery.permanent_failure`     | Counter    | `{failure}` | Counts outgoing deliveries abandoned as permanent failures.   |
-| `activitypub.delivery.duration`              | Histogram  | `ms`        | Measures outgoing ActivityPub delivery attempt duration.      |
-| `activitypub.inbox.processing_duration`      | Histogram  | `ms`        | Measures inbox listener processing duration.                  |
-| `activitypub.signature.verification_failure` | Counter    | `{failure}` | Counts failed HTTP Signature verification for inbox requests. |
+| Metric name                                  | Instrument | Unit        | Description                                                 |
+| -------------------------------------------- | ---------- | ----------- | ----------------------------------------------------------- |
+| `activitypub.delivery.sent`                  | Counter    | `{attempt}` | Counts outgoing ActivityPub delivery attempts.              |
+| `activitypub.delivery.permanent_failure`     | Counter    | `{failure}` | Counts outgoing deliveries abandoned as permanent failures. |
+| `activitypub.delivery.duration`              | Histogram  | `ms`        | Measures outgoing ActivityPub delivery attempt duration.    |
+| `activitypub.inbox.processing_duration`      | Histogram  | `ms`        | Measures inbox listener processing duration.                |
+| `activitypub.signature.verification_failure` | Counter    | `{failure}` | Counts failed signature verification for inbox requests.    |
 
 ### Metric attributes
 
diff --git a/packages/fedify/src/federation/context.ts b/packages/fedify/src/federation/context.ts
@@ -74,7 +74,7 @@ export interface Context<TContextData> {
    * The OpenTelemetry meter provider.
    * @since 2.3.0
    */
-  readonly meterProvider: MeterProvider;
+  readonly meterProvider?: MeterProvider;
 
   /**
    * The document loader for loading remote JSON-LD documents.
diff --git a/packages/fedify/src/federation/handler.test.ts b/packages/fedify/src/federation/handler.test.ts
@@ -3053,6 +3053,7 @@ test("handleInbox() nonce consumption on valid signed request", async () => {
 });
 
 test("handleInbox() nonce replay prevention", async () => {
+  const [meterProvider, recorder] = createTestMeterProvider();
   const activity = new Create({
     id: new URL("https://example.com/activities/nonce-3"),
     actor: new URL("https://example.com/person2"),
@@ -3075,7 +3076,10 @@ test("handleInbox() nonce replay prevention", async () => {
     rsaPublicKey3.id!,
     { spec: "rfc9421", rfc9421: { nonce } },
   );
-  const federation = createFederation<void>({ kv: new MemoryKvStore() });
+  const federation = createFederation<void>({
+    kv: new MemoryKvStore(),
+    meterProvider,
+  });
   const context = createRequestContext({
     federation,
     request: signedRequest,
@@ -3107,6 +3111,7 @@ test("handleInbox() nonce replay prevention", async () => {
     onNotFound: () => new Response("Not found", { status: 404 }),
     signatureTimeWindow: { minutes: 5 },
     skipSignatureVerification: false,
+    meterProvider,
     inboxChallengePolicy: {
       enabled: true,
       requestNonce: true,
@@ -3132,6 +3137,19 @@ test("handleInbox() nonce replay prevention", async () => {
     "no-store",
     "Challenge response must have Cache-Control: no-store",
   );
+  const failures = recorder.getMeasurements(
+    "activitypub.signature.verification_failure",
+  );
+  assertEquals(failures.length, 1);
+  assertEquals(failures[0].value, 1);
+  assertEquals(
+    failures[0].attributes["activitypub.remote.host"],
+    "example.com",
+  );
+  assertEquals(
+    failures[0].attributes["activitypub.verification.failure_reason"],
+    "invalidNonce",
+  );
 });
 
 test(
@@ -3273,6 +3291,7 @@ test(
 test(
   "handleInbox() actor/key mismatch does not consume nonce",
   async () => {
+    const [meterProvider, recorder] = createTestMeterProvider();
     // A request that has a valid RFC 9421 signature with a nonce, but the
     // signing key does not belong to the claimed actor.  The nonce must NOT be
     // consumed so the legitimate sender can still use it.
@@ -3304,7 +3323,10 @@ test(
       rsaPublicKey3.id!,
       { spec: "rfc9421", rfc9421: { nonce } },
     );
-    const federation = createFederation<void>({ kv: new MemoryKvStore() });
+    const federation = createFederation<void>({
+      kv: new MemoryKvStore(),
+      meterProvider,
+    });
     const context = createRequestContext({
       federation,
       request: maliciousRequest,
@@ -3336,6 +3358,7 @@ test(
       onNotFound: () => new Response("Not found", { status: 404 }),
       signatureTimeWindow: { minutes: 5 },
       skipSignatureVerification: false,
+      meterProvider,
       inboxChallengePolicy: {
         enabled: true,
         requestNonce: true,
@@ -3357,6 +3380,19 @@ test(
       true,
       "Nonce must not be consumed when actor/key ownership check fails",
     );
+    const failures = recorder.getMeasurements(
+      "activitypub.signature.verification_failure",
+    );
+    assertEquals(failures.length, 1);
+    assertEquals(failures[0].value, 1);
+    assertEquals(
+      failures[0].attributes["activitypub.remote.host"],
+      "example.com",
+    );
+    assertEquals(
+      failures[0].attributes["activitypub.verification.failure_reason"],
+      "actorKeyMismatch",
+    );
   },
 );
 
diff --git a/packages/fedify/src/federation/handler.ts b/packages/fedify/src/federation/handler.ts
@@ -1134,6 +1134,11 @@ async function handleInboxInternal<TContextData>(
   if (
     httpSigKey != null && !await doesActorOwnKey(activity, httpSigKey, ctx)
   ) {
+    getFederationMetrics(parameters.meterProvider)
+      .recordSignatureVerificationFailure(
+        "actorKeyMismatch",
+        httpSigKey.id?.hostname,
+      );
     logger.error(
       "The signer ({keyId}) and the actor ({actorId}) do not match.",
       {
@@ -1162,6 +1167,11 @@ async function handleInboxInternal<TContextData>(
       pendingNonceLabel,
     );
     if (!nonceValid) {
+      getFederationMetrics(parameters.meterProvider)
+        .recordSignatureVerificationFailure(
+          "invalidNonce",
+          httpSigKey?.id?.hostname,
+        );
       logger.error(
         "Signature nonce verification failed (missing, expired, or replayed).",
         { recipient },
diff --git a/packages/fedify/src/federation/metrics.ts b/packages/fedify/src/federation/metrics.ts
@@ -30,7 +30,7 @@ class FederationMetrics {
     this.signatureVerificationFailure = meter.createCounter(
       "activitypub.signature.verification_failure",
       {
-        description: "ActivityPub HTTP Signature verification failures.",
+        description: "ActivityPub signature verification failures.",
         unit: "{failure}",
       },
     );
diff --git a/packages/fedify/src/federation/middleware.test.ts b/packages/fedify/src/federation/middleware.test.ts
@@ -3657,6 +3657,30 @@ test("FederationImpl.processQueuedTask() permanent failure", async (t) => {
     },
   );
 
+  await t.step("malformed inbox does not break failure handling", async () => {
+    const [tracerProvider, exporter] = createTestTracerProvider();
+    const { federation, queuedMessages } = setup({ tracerProvider });
+
+    await federation.processQueuedTask(
+      undefined,
+      createOutboxMessage(
+        "not a url",
+        "https://example.com/activity/9",
+        ["https://gone.example/users/bob"],
+      ),
+    );
+
+    assertEquals(queuedMessages.length, 1);
+    assertEquals((queuedMessages[0] as OutboxMessage).attempt, 1);
+    const events = exporter.getEvents(
+      "activitypub.outbox",
+      "activitypub.delivery.failed",
+    );
+    assertEquals(events.length, 1);
+    assertEquals(events[0].attributes?.["activitypub.remote.host"], undefined);
+    assertEquals(events[0].attributes?.["activitypub.delivery.attempt"], 0);
+  });
+
   fetchMock.hardReset();
 });
 
diff --git a/packages/fedify/src/federation/middleware.ts b/packages/fedify/src/federation/middleware.ts
@@ -687,8 +687,20 @@ export class FederationImpl<TContextData>
       });
     } catch (error) {
       span.setStatus({ code: SpanStatusCode.ERROR, message: String(error) });
+      const remoteHost = (() => {
+        if (error instanceof SendActivityError) {
+          return getRemoteHost(error.inbox);
+        }
+        try {
+          return getRemoteHost(new URL(message.inbox));
+        } catch (_) {
+          return undefined;
+        }
+      })();
       span.addEvent("activitypub.delivery.failed", {
-        "activitypub.remote.host": getRemoteHost(new URL(message.inbox)),
+        ...(remoteHost == null
+          ? {}
+          : { "activitypub.remote.host": remoteHost }),
         "activitypub.delivery.attempt": message.attempt,
         "activitypub.delivery.permanent_failure":
           error instanceof SendActivityError &&
diff --git a/packages/fedify/src/federation/send.test.ts b/packages/fedify/src/federation/send.test.ts
@@ -498,14 +498,7 @@ test("sendActivity() records OpenTelemetry span events", async (t) => {
       event.attributes["activitypub.activity.id"],
       "https://example.com/activity",
     );
-    assert(typeof event.attributes["activitypub.activity.json"] === "string");
-
-    // Verify the JSON contains the activity
-    const recordedActivity = JSON.parse(
-      event.attributes["activitypub.activity.json"] as string,
-    );
-    assertEquals(recordedActivity.id, "https://example.com/activity");
-    assertEquals(recordedActivity.type, "Create");
+    assertEquals(event.attributes["activitypub.activity.json"], undefined);
 
     exporter.clear();
     fetchMock.hardReset();
diff --git a/packages/fedify/src/federation/send.ts b/packages/fedify/src/federation/send.ts
@@ -341,7 +341,6 @@ async function sendActivityInternal(
 
     // Record the sent activity with delivery details
     span.addEvent("activitypub.activity.sent", {
-      "activitypub.activity.json": JSON.stringify(activity),
       "activitypub.inbox.url": inbox.href,
       "activitypub.activity.id": activityId ?? "",
     });

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ class FederationMetrics {`
`30`	`30`	`this.signatureVerificationFailure = meter.createCounter(`
`31`	`31`	`"activitypub.signature.verification_failure",`
`32`	`32`	`{`
`33`		`- description: "ActivityPub HTTP Signature verification failures.",`
	`33`	`+ description: "ActivityPub signature verification failures.",`
`34`	`34`	`unit: "{failure}",`
`35`	`35`	`},`
`36`	`36`	`);`