OpenTelemetry: ActivityPub federation metrics#755
OpenTelemetry: ActivityPub federation metrics#755dahlia wants to merge 12 commits intofedify-dev:mainfrom
Conversation
Record OpenTelemetry metrics for delivery attempts, permanent delivery failures, signature verification failures, and inbox listener processing. Thread the meter provider through federation contexts so applications and tests can choose a provider explicitly. Refs: fedify-dev#619 Assisted-by: gpt-5.5
Describe the new meter provider option, emitted federation metrics, and the queued delivery failure span event. Update the changelog and overview copy so Fedify's OpenTelemetry support covers metrics as well as tracing. Refs: fedify-dev#619 Assisted-by: gpt-5.5
Add release notes and API documentation for the mock federation meter provider option exposed by @fedify/testing. Refs: fedify-dev#619 Assisted-by: gpt-5.5
dahlia
added
component/federation
|
@codex review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OpenTelemetry metrics support and wiring: new FederationMetrics, meter-provider option threaded through federation/context/send/inbox/handler/middleware, delivery-failure span-event emission, test meter utilities and noop providers, package/config updates, docs and changelog entries. ChangesOpenTelemetry metrics and wiring
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Fed as Federation.sendActivity
participant HTTP as Remote Inbox (HTTP)
participant Metrics as FederationMetrics
participant Meter as MeterProvider
App->>Fed: sendActivity(activity, meterProvider)
Fed->>Fed: start timer
Fed->>Metrics: getFederationMetrics(meterProvider)
Fed->>HTTP: POST activity to inbox
alt HTTP OK
HTTP-->>Fed: 2xx
Fed->>Metrics: recordDelivery(success=true, duration)
Metrics->>Meter: record counter/histogram
else HTTP Error
HTTP-->>Fed: error status
Fed->>Metrics: recordDelivery(success=false, duration)
Metrics->>Meter: record counter/histogram
alt Permanent failure
Fed->>Metrics: recordPermanentFailure(status)
Metrics->>Meter: record permanent_failure counter
end
Fed->>Fed: add activitypub.delivery.failed span event
end
sequenceDiagram
participant Queue as Activity Queue
participant Inbox as routeActivity()
participant Handler as Registered Listener
participant Metrics as FederationMetrics
participant Meter as MeterProvider
Queue->>Inbox: dispatch activity
Inbox->>Inbox: start timer
Inbox->>Handler: listener(context, activity)
Handler-->>Inbox: completes / throws
Inbox->>Inbox: compute duration
Inbox->>Metrics: recordInboxProcessingDuration(type, duration)
Metrics->>Meter: record histogram
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fedify/src/federation/context.ts`:
- Around line 73-77: The Context interface was changed to require meterProvider
which is a source-breaking minor release; make meterProvider optional on the
Context interface (e.g., change readonly meterProvider: MeterProvider to
readonly meterProvider?: MeterProvider) and ensure any internal creation path
(notably Federation.createContext()) and tests still supply a default
MeterProvider when absent, mirroring how tracerProvider was handled previously
so existing external implementations remain source-compatible.
In `@packages/fedify/src/federation/handler.ts`:
- Around line 998-1002: The signature verification failure metric is only
incremented in the verifyRequestDetailed() failure path; update the code to also
call
getFederationMetrics(parameters.meterProvider).recordSignatureVerificationFailure(reasonType,
remoteHost) in the other rejection paths that treat the request as a signature
verification failure—specifically in the doesActorOwnKey() mismatch branch and
in the deferred nonce-replay rejection branch—using the same shape of reason
data (type and keyId?.hostname) so all 401 signature-verification rejections are
counted consistently.
In `@packages/fedify/src/federation/metrics.ts`:
- Around line 30-36: Update the counter description for
signatureVerificationFailure to reflect both HTTP Signature and Object Integrity
Proof failures so it isn't misleading: locate the createCounter call that
defines signatureVerificationFailure in metrics.ts and change the description
from "ActivityPub HTTP Signature verification failures." to something like
"Signature verification failures (HTTP Signature and Object Integrity
Proof/OIP)." Ensure any references such as recordSignatureVerificationFailure
(called from handler.ts) remain consistent with the broader description.
In `@packages/fedify/src/federation/middleware.ts`:
- Around line 690-699: The span-event host extraction can throw inside the error
handler because new URL(message.inbox) is unguarded; update the catch-path
around span.addEvent in the code that logs "activitypub.delivery.failed" to
safely compute the host (use a small try/catch or validate message.inbox before
calling new URL) and pass either the host or undefined/fallback into
getRemoteHost so the span logging never throws. Locate the span.addEvent call
and the surrounding error handling (references: span.addEvent, getRemoteHost,
message.inbox, SendActivityError, permanentFailureStatusCodes) and ensure the
extraction is protected so retry and permanent-failure logic always run even if
message.inbox is malformed.
In `@packages/fedify/src/federation/send.test.ts`:
- Around line 515-657: Add assertions that the
"activitypub.delivery.permanent_failure" metric is recorded for
permanent-failure HTTP responses: locate the tests that call sendActivity (e.g.,
the steps labeled for failed responses and the 404/410 paths) and after the
sendActivity/assertRejects call use
recorder.getMeasurements("activitypub.delivery.permanent_failure") to assert
length === 1 (or expected count), value === 1, and attributes like
"activitypub.remote.host" and "activitypub.activity.type" match the test
activity; ensure you clear recorder/hardReset as done for other delivery
metrics.
In `@packages/fedify/src/federation/send.ts`:
- Around line 342-347: Remove the full serialized activity from span events to
avoid leaking user content and double-serialization: in the span.addEvent call
in send.ts (the "activitypub.activity.sent" event that currently references
JSON.stringify(activity)), stop adding "activitypub.activity.json"; keep only
"activitypub.inbox.url" and "activitypub.activity.id" for correlation, and if
you need the serialized body reuse the existing variable that was created
earlier when building the request body (the one at ~Line 263) rather than
calling JSON.stringify(activity) again; if you want payload details for
troubleshooting, only serialize and attach them on delivery failures (e.g., in
the error/failure span event) rather than on successful sends.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3951be5-d3cf-4393-964b-30336222058a
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
CHANGES.mddeno.jsondocs/manual/federation.mddocs/manual/opentelemetry.mddocs/why.mdpackages/fedify/package.jsonpackages/fedify/src/federation/context.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/handler.test.tspackages/fedify/src/federation/handler.tspackages/fedify/src/federation/inbox.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspackages/fedify/src/federation/send.test.tspackages/fedify/src/federation/send.tspackages/fedify/src/testing/context.tspackages/fixture/src/otel.tspackages/testing/src/context.tspackages/testing/src/mock.tspnpm-workspace.yaml
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry metrics support to Fedify, enabling the tracking of ActivityPub delivery attempts, permanent failures, inbox processing durations, and HTTP signature verification failures. It adds a new meterProvider option to createFederation() and exposes it via the Context object, while also providing new testing utilities like createTestMeterProvider and TestMetricRecorder in the fixture package. A critical issue was identified in packages/fedify/src/federation/send.ts where the activityType property is missing from the SendActivityParameters interface, which will result in a TypeScript compilation error since the property is already being destructured and used in the implementation.
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. fedify-dev#755 (comment) fedify-dev#755 (comment) fedify-dev#755 (comment) fedify-dev#755 (comment) fedify-dev#755 (comment) Assisted-by: gpt-5.5
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements OpenTelemetry metrics for ActivityPub delivery, inbox processing, and signature verification. It introduces the meterProvider configuration to createFederation() and the Context interface, supported by a new FederationMetrics utility. Testing infrastructure in @fedify/fixture and @fedify/testing has been expanded with TestMetricRecorder to facilitate metric assertions. Review feedback suggests updating the documentation to reflect the removal of the activity JSON from span events and the addition of new signature verification failure reasons. Additionally, it is recommended to use the getDurationMs helper consistently within the middleware for duration calculations.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/manual/opentelemetry.md (1)
233-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
activitypub.activity.sentdocs still reference a removed payload attribute.
activitypub.activity.sentno longer includesactivitypub.activity.json, but this page still documents it as present.📝 Suggested docs update
-| `activitypub.activity.sent` | `activitypub.send_activity` | Records full activity JSON and delivery details when an activity is sent. | +| `activitypub.activity.sent` | `activitypub.send_activity` | Records delivery details when an activity is sent. | ... **`activitypub.activity.sent` event attributes:** - - `activitypub.activity.json`: The complete activity JSON being sent - `activitypub.inbox.url`: The inbox URL where the activity was delivered - `activitypub.activity.id`: The activity IDAs per coding guidelines "docs/**/*.md: Update documentation when changing public APIs".
Also applies to: 264-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manual/opentelemetry.md` around lines 233 - 237, The docs claim the event activitypub.activity.sent contains the payload attribute activitypub.activity.json, but that attribute was removed; update the table rows for activitypub.activity.sent (and the related lines noted around the second instance) to remove any reference to activitypub.activity.json and instead document the current payload contents and recorded span name activitypub.send_activity (e.g., list the actual fields now emitted and any verification/delivery details). Ensure the two places referencing activitypub.activity.json (the table row for activitypub.activity.sent and the duplicate at lines ~264-267) are edited to reflect the new public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fedify/src/federation/send.test.ts`:
- Around line 589-605: The failure-path metrics assertions in send.test.ts are
missing checks for the "activitypub.remote.host" attribute; update the failure
step (after reading recorder.getMeasurements("activitypub.delivery.sent") into
sent and recorder.getMeasurements("activitypub.delivery.duration") into
durations) to assert that sent[0].attributes["activitypub.remote.host"] and
durations[0].attributes["activitypub.remote.host"] exist and equal the expected
remote host string (the same host used in the success assertions), so host
extraction is validated on the error path as well.
- Around line 589-605: The failure-step assertions in the test are missing
checks for the required attribute "activitypub.remote.host": after retrieving
sent via recorder.getMeasurements("activitypub.delivery.sent") and durations via
recorder.getMeasurements("activitypub.delivery.duration"), add assertions that
sent[0].attributes["activitypub.remote.host"] equals the expected host string
and that durations[0].attributes["activitypub.remote.host"] equals the same
host; update the block that follows fetchMock.hardReset() to include these two
checks so the error path validates host extraction the same way the success step
does.
---
Outside diff comments:
In `@docs/manual/opentelemetry.md`:
- Around line 233-237: The docs claim the event activitypub.activity.sent
contains the payload attribute activitypub.activity.json, but that attribute was
removed; update the table rows for activitypub.activity.sent (and the related
lines noted around the second instance) to remove any reference to
activitypub.activity.json and instead document the current payload contents and
recorded span name activitypub.send_activity (e.g., list the actual fields now
emitted and any verification/delivery details). Ensure the two places
referencing activitypub.activity.json (the table row for
activitypub.activity.sent and the duplicate at lines ~264-267) are edited to
reflect the new public API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd61bfae-d73b-4437-af81-9b0c39524e97
📒 Files selected for processing (9)
docs/manual/opentelemetry.mdpackages/fedify/src/federation/context.tspackages/fedify/src/federation/handler.test.tspackages/fedify/src/federation/handler.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.tspackages/fedify/src/federation/send.test.tspackages/fedify/src/federation/send.ts
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Use the shared duration helper for queued inbox processing metrics and cover remote host attributes on failed delivery metric assertions. fedify-dev#755 (comment) fedify-dev#755 (comment) fedify-dev#755 (comment) Assisted-by: gpt-5.5
|
/gemini review |
Update the ActivityPub send event description now that sent activity JSON is no longer recorded, and list the new verification failure reasons in the semantic attribute table. fedify-dev#755 (comment) fedify-dev#755 (comment) Assisted-by: gpt-5.5
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry metrics support to Fedify, enabling the monitoring of ActivityPub delivery attempts, permanent failures, and inbox processing durations. A new meterProvider option has been added to createFederation(), and the Context interface now exposes this provider to request, inbox, and outbox code. The implementation includes a new FederationMetrics utility for recording counters and histograms, along with testing helpers like createTestMeterProvider() and TestMetricRecorder to facilitate metric verification in runtime-agnostic tests. Documentation and dependency configurations have also been updated to support these observability enhancements. I have no feedback to provide.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/manual/opentelemetry.md`:
- Around line 236-237: The docs for the event activitypub.activity.sent are
inconsistent: update the event-attributes table under the "event-attributes"
section to match the summary that this event records delivery details (or vice
versa); specifically, replace or augment the activitypub.activity.json attribute
listed for activitypub.activity.sent with the delivery-related attributes used
by activitypub.send_activity (e.g., delivery details, recipient, status) so the
attributes listed under activitypub.activity.sent align with the description and
the activitypub.send_activity semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36e43949-8ef8-4a6e-a2cb-9107e4a6732b
📒 Files selected for processing (1)
docs/manual/opentelemetry.md
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry metrics support to Fedify, enabling the monitoring of ActivityPub delivery attempts, permanent failures, inbox processing durations, and HTTP signature verification failures. It adds a meterProvider option to createFederation() and updates the Context interface to expose this provider. The changes also include documentation updates and new testing utilities, such as TestMetricRecorder, for verifying metric measurements. Feedback suggests including the delivery success status as an attribute in the delivery duration histogram to allow for more granular analysis of performance profiles between successful and failed attempts.
Record delivery success on delivery duration metrics so successful and failed attempts can be separated in histograms, and remove the stale sent activity JSON attribute from the OpenTelemetry docs. fedify-dev#755 (comment) fedify-dev#755 (comment) Assisted-by: gpt-5.5
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements OpenTelemetry metrics for ActivityPub delivery, inbox processing, and signature verification, introducing a new meterProvider option to the Federation and Context APIs. It also enhances the testing framework with TestMetricRecorder for verifying metric measurements. The review feedback points out a critical missing property, activityType, in the SendActivityParameters interface which will lead to TypeScript compilation errors. Furthermore, the reviewer suggests refactoring the OTel meter initialization to avoid a dependency on deno.json metadata, ensuring better cross-runtime compatibility as per the project's style guidelines.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/manual/opentelemetry.md (1)
441-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale debug dashboard example — references the now-removed
activitypub.activity.jsonattribute.This PR removes
activitypub.activity.jsonfrom theactivitypub.activity.sentevent (confirmed in send.test.ts line 501 and the updated attribute list at lines 264–268). The outbound branch in the example code still reads that attribute and passes it toJSON.parse():activity: JSON.parse( event.attributes["activitypub.activity.json"] as string ),At runtime
event.attributes["activitypub.activity.json"]isundefined, soJSON.parse(undefined)throwsSyntaxError. Theactivityfield inActivityRecordalso can't be populated from the span event any more since the JSON is no longer recorded there — onlyactivitypub.inbox.urlandactivitypub.activity.idare available.📝 Proposed fix
if (event && event.attributes) { this.activities.push({ direction: "outbound", - activity: JSON.parse( - event.attributes["activitypub.activity.json"] as string - ), timestamp: new Date(span.startTime[0] * 1000), + activityId: event.attributes["activitypub.activity.id"] as string, + inboxUrl: event.attributes["activitypub.inbox.url"] as string, }); }You'll also need to update the
ActivityRecordinterface (and theactivity: unknownfield) to match the attributes that are actually available on theactivitypub.activity.sentevent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manual/opentelemetry.md` around lines 441 - 455, The example reads and JSON.parses the removed event attribute "activitypub.activity.json" causing a runtime SyntaxError; update the outbound branch that checks span.name === "activitypub.send_activity" (and the handling of event = span.events.find(...)) to stop accessing event.attributes["activitypub.activity.json"] and instead populate the ActivityRecord using the actual attributes available on the event (e.g. event.attributes["activitypub.inbox.url"] and event.attributes["activitypub.activity.id"]), remove the JSON.parse call, and update the ActivityRecord interface (and any usage of the activity field) to reflect the new shape (e.g. inboxUrl: string | undefined, activityId: string | undefined or activity: { id?: string; inboxUrl?: string } | undefined) so the code no longer expects the removed JSON attribute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/fedify/src/federation/send.test.ts`:
- Around line 624-666: The test "sendActivity() exports delivery metrics through
OpenTelemetry SDK" must wrap the cleanup steps in a try/finally so background
timers and global fetch mocks are always cleared; specifically, after creating
PeriodicExportingMetricReader and MeterProvider and calling sendActivity and
meterProvider.forceFlush(), move meterProvider.shutdown() and
fetchMock.hardReset() into a finally block (paired with a try that surrounds the
assertions) so the MeterProvider.shutdown() and fetchMock.hardReset() always run
even if assertions fail.
---
Outside diff comments:
In `@docs/manual/opentelemetry.md`:
- Around line 441-455: The example reads and JSON.parses the removed event
attribute "activitypub.activity.json" causing a runtime SyntaxError; update the
outbound branch that checks span.name === "activitypub.send_activity" (and the
handling of event = span.events.find(...)) to stop accessing
event.attributes["activitypub.activity.json"] and instead populate the
ActivityRecord using the actual attributes available on the event (e.g.
event.attributes["activitypub.inbox.url"] and
event.attributes["activitypub.activity.id"]), remove the JSON.parse call, and
update the ActivityRecord interface (and any usage of the activity field) to
reflect the new shape (e.g. inboxUrl: string | undefined, activityId: string |
undefined or activity: { id?: string; inboxUrl?: string } | undefined) so the
code no longer expects the removed JSON attribute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d38778b9-4411-4a35-a729-aceb17897792
📒 Files selected for processing (3)
docs/manual/opentelemetry.mdpackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/send.test.ts
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The debug exporter example now records the outbound activity ID and inbox URL from the sent activity event attributes instead of parsing the removed activity JSON event attribute. Assisted-by: gpt-5.5
Add fallback metadata for the federation meter and make the SDK metrics test always release its meter provider and fetch mock, even when assertions fail. fedify-dev#755 (comment) fedify-dev#755 (comment) Assisted-by: gpt-5.5
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry metrics support to Fedify, enabling the monitoring of ActivityPub delivery attempts, permanent failures, inbox processing durations, and HTTP signature verification failures. It adds a meterProvider option to createFederation(), updates the Context interface, and provides new testing utilities like TestMetricRecorder. The reviewer feedback focuses on improving maintainability and consistency by using the centralized getRemoteHost() helper function for hostname extraction in telemetry payloads within handler.ts.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Route signature verification failure metrics through the shared remote host helper so inbox rejection paths use the same host extraction as delivery metrics. fedify-dev#755 (comment) fedify-dev#755 (comment) fedify-dev#755 (comment) Assisted-by: gpt-5.5
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements OpenTelemetry metrics for ActivityPub delivery, inbox processing, and signature verification, while introducing a meterProvider configuration option and exposing it via the Context. It also adds the activitypub.delivery.failed span event and provides new testing helpers in @fedify/fixture for metric assertions. Review feedback suggests maintaining consistency by keeping activity JSON in span events, ensuring cross-runtime compatibility for JSON imports, and adding warning logs for malformed inbox URLs during error handling.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Queue messages with invalid inbox URLs already keep failure telemetry from crashing. Log the malformed inbox too, so operators can identify corrupted outbox messages. fedify-dev#755 (comment) Assisted-by: gpt-5.5
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry metrics support to Fedify, enabling monitoring of ActivityPub delivery attempts, permanent failures, inbox processing duration, and signature verification errors. It adds a meterProvider option to createFederation() and exposes it through the Context interface. Additionally, new testing helpers like createTestMeterProvider() and TestMetricRecorder are added to @fedify/fixture. Feedback identifies a cross-runtime compatibility issue when importing deno.json in metrics.ts and notes that the removal of the activitypub.activity.json attribute from span events should be documented as a breaking change.
| type MeterProvider, | ||
| metrics, | ||
| } from "@opentelemetry/api"; | ||
| import metadata from "../../deno.json" with { type: "json" }; |
There was a problem hiding this comment.
Importing deno.json directly into source code can cause runtime errors in Node.js or Bun environments if the file is not included in the published npm package or if the environment does not support JSON modules with the with syntax (e.g., older versions of Node.js). This violates the project's goal of being framework-agnostic and working across Deno, Node.js, and Bun. Consider hardcoding the package name or using a different mechanism to provide the version that is compatible with all target runtimes.
References
- For code intended to run in both Deno and Node.js environments, prefer Node.js APIs over Deno-specific APIs to ensure cross-runtime compatibility.
| span.addEvent("activitypub.activity.sent", { | ||
| "activitypub.inbox.url": inbox.href, | ||
| "activitypub.activity.id": activityId ?? "", | ||
| }); |
There was a problem hiding this comment.
The activitypub.activity.json attribute has been removed from the activitypub.activity.sent span event. This is a breaking change for users who rely on this attribute for debugging or auditing sent activities. While removing it may improve performance and reduce trace size, this change is not mentioned in the PR description or CHANGES.md. If this removal is intentional, it should be documented as a breaking change.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
This adds OpenTelemetry metrics for core ActivityPub federation paths, covering outbound delivery attempts, permanent delivery failures, HTTP Signature verification failures, and inbox listener processing duration.
Closes #619.
Changes
meterProviderthrough federation options, contexts, send handling, inbox routing, and queued federation work.createTestMeterProvider()andTestMetricRecorderto @fedify/fixture for runtime-agnostic metric assertions.Notes
@opentelemetry/sdk-metricsis only used by tests, so it is listed as a development dependency for @fedify/fedify. Runtime code only depends on@opentelemetry/api.All newly exported APIs added by this PR include
@since 2.3.0.Verification
mise check && mise test:deno && mise test:nodemise docs:build