Add cooperative benchmark mode for federation targets#787
Conversation
|
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 cooperative ChangesCooperative Benchmark Mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![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 a cooperative benchmarking mode (benchmarkMode) to Fedify, exposing endpoints for in-process OpenTelemetry metric snapshots (/.well-known/fedify/bench/stats) and activity triggers (/.well-known/fedify/bench/trigger). It also adds a new fedify.queue.depth gauge to track queue depth and updates signature verification histograms with low-latency buckets. The reviewer feedback suggests wrapping queue.getDepth() in a try-catch block with a defensive null check to ensure robust metric collection, and wrapping request.json() in a local try-catch block to cleanly handle malformed or empty request bodies with a 400 response instead of a 500 error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 137-147: Add a new focused test that verifies
authenticatedDocumentLoaderFactory does not disable the benchmark-mode default:
call createFederation with benchmarkMode: true and only
authenticatedDocumentLoaderFactory set (omit documentLoaderFactory and
contextLoaderFactory), then assert the returned instance is a FederationImpl and
that federation.allowPrivateAddress === true; place this alongside the existing
"benchmarkMode leaves custom loader factories alone" test to catch regressions
where authenticatedDocumentLoaderFactory alone improperly flips the benchmark
default.
🪄 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: b9cc7233-2974-46c7-8695-dff609719cf2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
CHANGES.mddocs/.vitepress/config.mtsdocs/manual/benchmarking.mddocs/manual/federation.mddocs/manual/opentelemetry.mdpackages/fedify/package.jsonpackages/fedify/src/federation/bench.tspackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/metrics.test.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.ts
Benchmark targets need a cooperative mode that enables safe-by-default benchmarking relaxations without changing normal federation behavior. This adds the public option and threads it through both createFederation() and FederationBuilder.build(), while preserving explicit option precedence and custom loader behavior. fedify-dev#782 Assisted-by: Codex:gpt-5.5
The benchmark CLI needs target-side metrics without requiring a separate OpenTelemetry backend. This adds the benchmark-owned in-process reader, the stats endpoint, queue depth observations, and explicit signature verification buckets for low-latency measurements. fedify-dev#782 Assisted-by: Codex:gpt-5.5
Benchmark clients need a target-side hook for driving application sendActivity() paths while keeping recipients constrained to known sink inboxes by default. This adds the trigger endpoint, request validation, sink-list enforcement, and explicit unsafe-recipient override for benchmark-only runs. fedify-dev#782 Assisted-by: Codex:gpt-5.5
Benchmark mode introduces target-side endpoint contracts and metric behavior that users need to configure intentionally. This adds manual documentation, OpenTelemetry metric references, sidebar navigation, and the 2.3.0 changelog entry for the cooperative benchmark endpoints. fedify-dev#744 fedify-dev#782 Assisted-by: Codex:gpt-5.5
Queue depth is documented as a regular Fedify OpenTelemetry metric, not as a benchmark-only signal. Register the observable gauge after selecting the meter provider so normal applications with getDepth-capable queues can export it without enabling benchmark endpoints. Add a regression test that constructs a regular federation with an explicit MeterProvider and verifies fedify.queue.depth is collected. fedify-dev#782 Assisted-by: Codex:gpt-5.5
Benchmark mode owns its in-process metric reader, so application-provided MeterProvider instances cannot be combined with benchmarkMode: true. Make that TypeError contract visible on createFederation() and FederationBuilder.build(). Also document the common deployment pattern where one code path switches benchmark mode with an environment flag and passes the application meter provider only when benchmark mode is disabled. fedify-dev#782 Assisted-by: Codex:gpt-5.5
dahlia
force-pushed
the
feat/bench/benchmark-mode
branch
from
fd77875 to
48aed08
Compare
|
@codex review |
dahlia
moved this to In review
in STF 2025–2026 Milestone 6: Observability and Monitoring (OpenTelemetry)
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48aed08935
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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 benchmark mode change moved @opentelemetry/sdk-metrics into the @fedify/fedify runtime dependencies, but the lockfile still listed the specifier under @fedify/debugger. Frozen pnpm installs therefore failed before CI could run any checks. Move the importer entry to match the package manifests. fedify-dev#787 Assisted-by: Codex:gpt-5.5
Queue depth observation is best-effort because third-party queue backends may fail while reporting their depth. Skip failed or missing snapshots so one backend cannot poison the whole OpenTelemetry collection pass. fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
Some runtimes throw non-SyntaxError exceptions while parsing invalid or empty JSON request bodies. Parse the trigger body in its own guarded step so those requests consistently receive a 400 response. fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
An authenticated document loader only affects authenticated key fetching. It should not disable benchmarkMode's private-address default for the regular document and context loaders, whose local benchmark behavior is still needed. Keep the explicit allowPrivateAddress and userAgent guard intact while avoiding that guard for the benchmark default. fedify-dev#787 (comment) fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cooperative benchmarkMode to createFederation() and FederationBuilder.build(). When enabled, it exposes local benchmark endpoints (/.well-known/fedify/bench/stats and /.well-known/fedify/bench/trigger), configures an in-process OpenTelemetry metrics reader, and relaxes certain defaults (such as allowing private addresses and disabling signature time windows). It also introduces a new fedify.queue.depth gauge to report queue depth. Feedback on the changes suggests avoiding the use of as never to bypass type checking for mocks in the metrics tests, recommending a direct return of null instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ 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". |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/bench.ts`:
- Around line 139-149: The returned triggerId is generated but never attached to
the outbound activity or any queued/emitted telemetry, so it cannot be used for
correlation; update the handler to include triggerId in the outbound path by
adding it to the activity (e.g., set activity.id, activity.properties.triggerId,
or activity.context.triggerId) before calling context.sendActivity(sender,
recipients, activity) and ensure any queuing/metrics/emit functions include that
same triggerId, or alternatively remove triggerId from the jsonResponse payload;
adjust the code around the triggerId declaration, context.sendActivity call, and
jsonResponse call (and any send/queue/emit helpers) so the ID is either
propagated with the activity or eliminated from the response.
In `@packages/fedify/src/federation/builder.test.ts`:
- Around line 171-178: Add a second call to builder.build() that explicitly sets
benchmarkMode: true together with explicit allowPrivateAddress and
signatureTimeWindow values, then cast to FederationImpl and assert the explicit
values take precedence (e.g., impl.benchmarkMode is true and
impl.allowPrivateAddress / impl.signatureTimeWindow reflect the passed
overrides). Locate the existing assertions around builder.build, the
FederationImpl cast (impl), and add the new build+assert block to verify
explicit options override implicit benchmark defaults.
In `@packages/fedify/src/federation/federation.ts`:
- Around line 945-947: Update the JSDoc on FederationOptions in federation.ts so
it accurately states that the default of allowPrivateAddress switching to true
(and signatureTimeWindow defaulting to false) is suppressed not only when a
custom document loader is provided but also when a contextLoaderFactory is
supplied; specifically reference FederationOptions.allowPrivateAddress,
FederationOptions.signatureTimeWindow, documentLoaderFactory and
contextLoaderFactory in the description so the public comments match the runtime
behavior.
In `@packages/fedify/src/federation/metrics.test.ts`:
- Around line 100-133: The test "signature verification duration uses explicit
low-latency buckets" must always call meterProvider.shutdown() even if
assertions fail; wrap the body that uses reader, meterProvider,
getFederationMetrics(...).recordSignatureVerificationDuration(...) and the
assertions in a try block and move await meterProvider.shutdown() into a finally
block so the MeterProvider is always torn down and the TestMetricReader
unregistered.
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 8016-8047: The test "expired held activity is dropped" relies on
the real clock; freeze or mock the clock so now is deterministic (e.g., set now
to a fixed instant > "2026-05-25T00:00:00Z", such as "2026-05-25T00:00:02Z")
before calling setup() / federation.processQueuedTask and restore the clock
afterward so the heldActivityTtl (1s) deterministically expires; use your
project's existing fake-timer utility (or Temporal.Now instant override /
sinon.useFakeTimers) around the calls to setup() and
federation.processQueuedTask to ensure the assertion comparing dropped.heldSince
and permanentFailureReason is stable.
In `@packages/fedify/src/federation/middleware.ts`:
- Around line 488-492: The benchmark warning currently prints a generic message
when benchmarkMode is true; change it to compute which protections are actually
relaxed by inspecting the effective values of allowPrivateAddress and
signatureTimeWindow (the computed variables used by the middleware) and log a
clear, specific warning listing each disabled or reduced protection (e.g.,
"private address checks disabled" if allowPrivateAddress is true, and "signature
time window extended to Xs" if signatureTimeWindow differs from the secure
default). Use the same logger (getLogger(["fedify","federation","benchmark"]))
and include the concrete effective values in the message so operators can see
exactly what protections were relaxed.
🪄 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: 9b82e97f-0c54-4983-9d46-df887e6bbfcd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
CHANGES.mddocs/.vitepress/config.mtsdocs/manual/benchmarking.mddocs/manual/federation.mddocs/manual/opentelemetry.mdpackages/fedify/package.jsonpackages/fedify/src/federation/bench.tspackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/metrics.test.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🤖 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/bench.ts`:
- Around line 139-149: The returned triggerId is generated but never attached to
the outbound activity or any queued/emitted telemetry, so it cannot be used for
correlation; update the handler to include triggerId in the outbound path by
adding it to the activity (e.g., set activity.id, activity.properties.triggerId,
or activity.context.triggerId) before calling context.sendActivity(sender,
recipients, activity) and ensure any queuing/metrics/emit functions include that
same triggerId, or alternatively remove triggerId from the jsonResponse payload;
adjust the code around the triggerId declaration, context.sendActivity call, and
jsonResponse call (and any send/queue/emit helpers) so the ID is either
propagated with the activity or eliminated from the response.
In `@packages/fedify/src/federation/builder.test.ts`:
- Around line 171-178: Add a second call to builder.build() that explicitly sets
benchmarkMode: true together with explicit allowPrivateAddress and
signatureTimeWindow values, then cast to FederationImpl and assert the explicit
values take precedence (e.g., impl.benchmarkMode is true and
impl.allowPrivateAddress / impl.signatureTimeWindow reflect the passed
overrides). Locate the existing assertions around builder.build, the
FederationImpl cast (impl), and add the new build+assert block to verify
explicit options override implicit benchmark defaults.
In `@packages/fedify/src/federation/federation.ts`:
- Around line 945-947: Update the JSDoc on FederationOptions in federation.ts so
it accurately states that the default of allowPrivateAddress switching to true
(and signatureTimeWindow defaulting to false) is suppressed not only when a
custom document loader is provided but also when a contextLoaderFactory is
supplied; specifically reference FederationOptions.allowPrivateAddress,
FederationOptions.signatureTimeWindow, documentLoaderFactory and
contextLoaderFactory in the description so the public comments match the runtime
behavior.
In `@packages/fedify/src/federation/metrics.test.ts`:
- Around line 100-133: The test "signature verification duration uses explicit
low-latency buckets" must always call meterProvider.shutdown() even if
assertions fail; wrap the body that uses reader, meterProvider,
getFederationMetrics(...).recordSignatureVerificationDuration(...) and the
assertions in a try block and move await meterProvider.shutdown() into a finally
block so the MeterProvider is always torn down and the TestMetricReader
unregistered.
In `@packages/fedify/src/federation/middleware.test.ts`:
- Around line 8016-8047: The test "expired held activity is dropped" relies on
the real clock; freeze or mock the clock so now is deterministic (e.g., set now
to a fixed instant > "2026-05-25T00:00:00Z", such as "2026-05-25T00:00:02Z")
before calling setup() / federation.processQueuedTask and restore the clock
afterward so the heldActivityTtl (1s) deterministically expires; use your
project's existing fake-timer utility (or Temporal.Now instant override /
sinon.useFakeTimers) around the calls to setup() and
federation.processQueuedTask to ensure the assertion comparing dropped.heldSince
and permanentFailureReason is stable.
In `@packages/fedify/src/federation/middleware.ts`:
- Around line 488-492: The benchmark warning currently prints a generic message
when benchmarkMode is true; change it to compute which protections are actually
relaxed by inspecting the effective values of allowPrivateAddress and
signatureTimeWindow (the computed variables used by the middleware) and log a
clear, specific warning listing each disabled or reduced protection (e.g.,
"private address checks disabled" if allowPrivateAddress is true, and "signature
time window extended to Xs" if signatureTimeWindow differs from the secure
default). Use the same logger (getLogger(["fedify","federation","benchmark"]))
and include the concrete effective values in the message so operators can see
exactly what protections were relaxed.
🪄 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: 9b82e97f-0c54-4983-9d46-df887e6bbfcd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
CHANGES.mddocs/.vitepress/config.mtsdocs/manual/benchmarking.mddocs/manual/federation.mddocs/manual/opentelemetry.mdpackages/fedify/package.jsonpackages/fedify/src/federation/bench.tspackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/metrics.test.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.ts
🛑 Comments failed to post (7)
packages/fedify/src/federation/bench.ts (1)
139-149:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
triggerIddoes not correlate anything yet.This ID is generated and returned, but it never gets attached to
context.sendActivity(), queued work, or any emitted metric. A client polling/.well-known/fedify/bench/statstherefore has no server-side signal to match back to this trigger, so concurrent benchmark runs cannot use it as a real correlation handle. Please either plumb the ID through the outbound path or drop it from the response contract until it is actionable.🤖 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 `@packages/fedify/src/federation/bench.ts` around lines 139 - 149, The returned triggerId is generated but never attached to the outbound activity or any queued/emitted telemetry, so it cannot be used for correlation; update the handler to include triggerId in the outbound path by adding it to the activity (e.g., set activity.id, activity.properties.triggerId, or activity.context.triggerId) before calling context.sendActivity(sender, recipients, activity) and ensure any queuing/metrics/emit functions include that same triggerId, or alternatively remove triggerId from the jsonResponse payload; adjust the code around the triggerId declaration, context.sendActivity call, and jsonResponse call (and any send/queue/emit helpers) so the ID is either propagated with the activity or eliminated from the response.packages/fedify/src/federation/builder.test.ts (1)
171-178: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Cover explicit benchmark override precedence too.
This only verifies the implicit benchmark defaults. Please add a second build in this step that passes
benchmarkMode: truewith explicitallowPrivateAddress/signatureTimeWindowvalues and assert those win, otherwise the main precedence contract can regress without this test catching it.🤖 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 `@packages/fedify/src/federation/builder.test.ts` around lines 171 - 178, Add a second call to builder.build() that explicitly sets benchmarkMode: true together with explicit allowPrivateAddress and signatureTimeWindow values, then cast to FederationImpl and assert the explicit values take precedence (e.g., impl.benchmarkMode is true and impl.allowPrivateAddress / impl.signatureTimeWindow reflect the passed overrides). Locate the existing assertions around builder.build, the FederationImpl cast (impl), and add the new build+assert block to verify explicit options override implicit benchmark defaults.packages/fedify/src/federation/federation.ts (1)
945-947:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument that
contextLoaderFactoryalso suppresses the benchmark default.This JSDoc says
allowPrivateAddressflips totrueunless a custom document loader is configured, but the implementation also disables that default whencontextLoaderFactoryis provided. Please mention both factories so the public contract matches runtime behavior.Suggested wording
- * When enabled, {`@link` FederationOptions.allowPrivateAddress} defaults to - * `true` unless a custom document loader is configured, and + * When enabled, {`@link` FederationOptions.allowPrivateAddress} defaults to + * `true` unless a custom document loader or context loader is configured, + * and📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.* When enabled, {`@link` FederationOptions.allowPrivateAddress} defaults to * `true` unless a custom document loader or context loader is configured, * and * {`@link` FederationOptions.signatureTimeWindow} defaults to `false`.🤖 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 `@packages/fedify/src/federation/federation.ts` around lines 945 - 947, Update the JSDoc on FederationOptions in federation.ts so it accurately states that the default of allowPrivateAddress switching to true (and signatureTimeWindow defaulting to false) is suppressed not only when a custom document loader is provided but also when a contextLoaderFactory is supplied; specifically reference FederationOptions.allowPrivateAddress, FederationOptions.signatureTimeWindow, documentLoaderFactory and contextLoaderFactory in the description so the public comments match the runtime behavior.packages/fedify/src/federation/metrics.test.ts (1)
100-133: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Wrap the
MeterProvidershutdown infinally.If an assertion fails before Line 132, this test skips
meterProvider.shutdown()and leaves the reader registered for the rest of the file. Please mirror thetry { ... } finally { await meterProvider.shutdown(); }pattern you already used below.Suggested change
test("signature verification duration uses explicit low-latency buckets", async () => { const reader = new TestMetricReader(); const meterProvider = new MeterProvider({ readers: [reader] }); - getFederationMetrics(meterProvider).recordSignatureVerificationDuration( - 7, - "http", - "verified", - ); - - const result = await reader.collect(); - const metric = result.resourceMetrics.scopeMetrics - .flatMap((scope) => scope.metrics) - .find((metric) => - metric.descriptor.name === "activitypub.signature.verification.duration" - ); - assertEquals(metric?.dataPointType, DataPointType.HISTOGRAM); - const histogram = metric as HistogramMetricData | undefined; - assertEquals(histogram?.dataPoints[0].value.buckets.boundaries, [ - 0.1, - 0.25, - 0.5, - 1, - 2.5, - 5, - 10, - 25, - 50, - 100, - 250, - 500, - 1000, - ]); - await meterProvider.shutdown(); + try { + getFederationMetrics(meterProvider).recordSignatureVerificationDuration( + 7, + "http", + "verified", + ); + + const result = await reader.collect(); + const metric = result.resourceMetrics.scopeMetrics + .flatMap((scope) => scope.metrics) + .find((metric) => + metric.descriptor.name === "activitypub.signature.verification.duration" + ); + assertEquals(metric?.dataPointType, DataPointType.HISTOGRAM); + const histogram = metric as HistogramMetricData | undefined; + assertEquals(histogram?.dataPoints[0].value.buckets.boundaries, [ + 0.1, + 0.25, + 0.5, + 1, + 2.5, + 5, + 10, + 25, + 50, + 100, + 250, + 500, + 1000, + ]); + } finally { + await meterProvider.shutdown(); + } });Based on learnings: in this Deno/TypeScript codebase, structure tests as
try { ... } finally { ... }so teardown runs regardless of how the test exits.🤖 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 `@packages/fedify/src/federation/metrics.test.ts` around lines 100 - 133, The test "signature verification duration uses explicit low-latency buckets" must always call meterProvider.shutdown() even if assertions fail; wrap the body that uses reader, meterProvider, getFederationMetrics(...).recordSignatureVerificationDuration(...) and the assertions in a try block and move await meterProvider.shutdown() into a finally block so the MeterProvider is always torn down and the TestMetricReader unregistered.packages/fedify/src/federation/metrics.ts (1)
1208-1252:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid stacking multiple queue-depth callbacks on one
MeterProvider.
getFederationMetrics()caches a singlequeueDepthinstrument per provider, but this function always adds a fresh callback that captures its ownuniqueQueues. If two federation instances share the sameMeterProvider—or the same provider is rebuilt—you'll emit duplicate/stalefedify.queue.depthpoints on every collection. Please move the observed-queue registry into provider-scoped state and attach only one callback per provider.packages/fedify/src/federation/middleware.test.ts (1)
8016-8047:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFreeze the clock in this TTL-drop test.
This step still depends on the real clock: it hard-codes
2026-05-25T00:00:00ZforcircuitHeldSince, then expects the activity to be expired without injectingnow. That makes the assertion time-dependent instead of deterministic.🛠️ Suggested fix
await t.step("expired held activity is dropped", async () => { fetchMock.hardReset(); fetchMock.spyGlobal(); + const heldSince = Temporal.Instant.from("2026-05-25T00:00:00Z"); + const now = Temporal.Instant.from("2026-05-25T00:00:02Z"); let dropped: { remoteHost: string; heldSince: Temporal.Instant } | null = null; - const { federation, queued } = setup({ + const { federation, queued, kv } = setup({ failureThreshold: 1, heldActivityTtl: { seconds: 1 }, onActivityDrop(remoteHost, details) { dropped = { remoteHost, heldSince: details.heldSince }; }, }); + federation.circuitBreaker = new CircuitBreaker({ + kv, + prefix: ["_fedify", "circuit"], + now: () => now, + options: { + failureThreshold: 1, + heldActivityTtl: { seconds: 1 }, + onActivityDrop(remoteHost, details) { + dropped = { remoteHost, heldSince: details.heldSince }; + }, + }, + }); let permanentFailureReason: unknown; federation.setOutboxPermanentFailureHandler((_ctx, values) => { permanentFailureReason = values.reason; }); await federation.processQueuedTask( undefined, createOutboxMessage("https://ttl.example/inbox", { circuitHeld: true, - circuitHeldSince: "2026-05-25T00:00:00Z", + circuitHeldSince: heldSince.toString(), }), ); assertEquals(queued, []); assertEquals(dropped, { remoteHost: "ttl.example", - heldSince: Temporal.Instant.from("2026-05-25T00:00:00Z"), + heldSince, }); assertEquals(permanentFailureReason, "circuit-breaker-ttl"); });🤖 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 `@packages/fedify/src/federation/middleware.test.ts` around lines 8016 - 8047, The test "expired held activity is dropped" relies on the real clock; freeze or mock the clock so now is deterministic (e.g., set now to a fixed instant > "2026-05-25T00:00:00Z", such as "2026-05-25T00:00:02Z") before calling setup() / federation.processQueuedTask and restore the clock afterward so the heldActivityTtl (1s) deterministically expires; use your project's existing fake-timer utility (or Temporal.Now instant override / sinon.useFakeTimers) around the calls to setup() and federation.processQueuedTask to ensure the assertion comparing dropped.heldSince and permanentFailureReason is stable.packages/fedify/src/federation/middleware.ts (1)
488-492:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the benchmark warning name the effective disabled protections.
This log never tells operators what benchmark mode is actually relaxing. Worse, explicit overrides can keep one or both safeguards enabled, so the warning should be derived from the computed
allowPrivateAddress/signatureTimeWindowvalues rather than a generic “relaxations are active” message.Suggested direction
if (benchmarkMode) { - getLogger(["fedify", "federation", "benchmark"]).warn( - "Fedify benchmarkMode is enabled; benchmark-only relaxations are " + - "active and must not be used in production.", - ); + const disabledChecks = [ + allowPrivateAddress + ? "SSRF protections for the built-in loaders" + : null, + signatureTimeWindow === false + ? "signature-freshness checks" + : null, + ].filter((value): value is string => value != null); + getLogger(["fedify", "federation", "benchmark"]).warn( + disabledChecks.length > 0 + ? "Fedify benchmarkMode is enabled; " + + `${disabledChecks.join(" and ")} ` + + "are disabled and must not be used in production." + : "Fedify benchmarkMode is enabled; benchmark endpoints are active " + + "and must not be used in production.", + ); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (benchmarkMode) { const disabledChecks = [ allowPrivateAddress ? "SSRF protections for the built-in loaders" : null, signatureTimeWindow === false ? "signature-freshness checks" : null, ].filter((value): value is string => value != null); getLogger(["fedify", "federation", "benchmark"]).warn( disabledChecks.length > 0 ? "Fedify benchmarkMode is enabled; " + `${disabledChecks.join(" and ")} ` + "are disabled and must not be used in production." : "Fedify benchmarkMode is enabled; benchmark endpoints are active " + "and must not be used in production.", ); }🤖 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 `@packages/fedify/src/federation/middleware.ts` around lines 488 - 492, The benchmark warning currently prints a generic message when benchmarkMode is true; change it to compute which protections are actually relaxed by inspecting the effective values of allowPrivateAddress and signatureTimeWindow (the computed variables used by the middleware) and log a clear, specific warning listing each disabled or reduced protection (e.g., "private address checks disabled" if allowPrivateAddress is true, and "signature time window extended to Xs" if signatureTimeWindow differs from the secure default). Use the same logger (getLogger(["fedify","federation","benchmark"])) and include the concrete effective values in the message so operators can see exactly what protections were relaxed.
Remove the benchmark trigger ID from the response because it is not propagated through the outbound activity path. Clarify benchmark-mode defaults around loader factories, verify explicit overrides, and make the benchmark warning name the effective relaxed protections while keeping the LogTape properties structured for machine-readable logging. Use a deterministic circuit breaker clock for the expired held activity test so the TTL assertion does not depend on wall-clock time. fedify-dev#787 Assisted-by: Codex:gpt-5.5
There was a problem hiding this comment.
Code Review
This pull request introduces a cooperative benchmarkMode to Fedify, exposing endpoints under /.well-known/fedify/bench/ for metric snapshots and activity triggers, relaxing security defaults for benchmark targets, and adding a new fedify.queue.depth gauge. The feedback recommends validating that recipient inboxes are not empty in the trigger endpoint to avoid silent no-ops, and optimizing performance by querying queue depths and parsing recipients in parallel using Promise.all instead of sequential loops.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c8d3d930d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
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 `@packages/fedify/src/federation/middleware.ts`:
- Around line 254-270: The code calls
Temporal.Duration.from(signatureTimeWindow).total({ unit: "seconds" }) which can
throw for calendar-aware durations; wrap that conversion in a try/catch around
the Temporal.Duration.from(...).total(...) call in the same branch that handles
signatureTimeWindow !== false, and only push the "http_signature_time_window"
relaxations with effect "changed" when the conversion succeeds and seconds !==
3600; on catch, skip adding the "changed" relaxation (leaving the rest of the
warning logic intact) so benchmark/ warning generation won’t throw.
🪄 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: f33d1917-8e51-486f-bdfc-cc39e339f59a
📒 Files selected for processing (7)
docs/manual/benchmarking.mdpackages/fedify/src/federation/bench.tspackages/fedify/src/federation/builder.test.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/metrics.test.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.ts
💤 Files with no reviewable changes (2)
- docs/manual/benchmarking.md
- packages/fedify/src/federation/bench.ts
Make benchmark trigger delivery use server-configured sink inboxes instead of trusting allowlists or bypass flags from the request body. This keeps the trigger endpoint from being used to make a target sign and enqueue traffic to arbitrary inboxes. Also make benchmark warning generation tolerate calendar-aware signature time windows by skipping the seconds summary when Temporal cannot convert the value without a relative reference. fedify-dev#787 (comment) fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
Collect queue depth snapshots in parallel so a slow queue backend does not block other queue depth observations during an OpenTelemetry collection pass. Add a regression test that verifies the fast queue starts while another queue is still pending. fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
Reject trigger requests that produce no recipient inboxes so benchmark clients get immediate feedback instead of a successful no-op. Parse recipients in parallel as part of the trigger request so remote document or context work does not serialize across recipients. fedify-dev#787 (comment) fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces a cooperative benchmarking mode (benchmarkMode) to Fedify. When enabled, it exposes local benchmark endpoints under /.well-known/fedify/bench/ (specifically stats for in-process OpenTelemetry metric snapshots and trigger for driving sendActivity() to configured benchmark sinks). It also relaxes certain security defaults (such as allowing private addresses and disabling signature time windows) for testing purposes, and adds a new fedify.queue.depth gauge to track queue backlogs. Extensive documentation and unit tests have been added to support this feature. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/fedify/src/federation/bench.ts (2)
28-87: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSDoc to the new benchmark exports.
Most of the new exported benchmark surface here is undocumented (
createBenchmarkMeterProvider(), the snapshot/data interfaces,collectBenchmarkMetrics(),handleBenchmarkStats(),handleBenchmarkTrigger(), andBenchmarkTriggerOptions). This file lives under packages/fedify/src/, so these public APIs should ship with JSDoc like the rest of the federation surface.As per coding guidelines,
**/src/**/*.ts: Include JSDoc comments for public APIs.Also applies to: 90-167
🤖 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 `@packages/fedify/src/federation/bench.ts` around lines 28 - 87, Public exports in this file lack JSDoc; add short JSDoc comments for each exported symbol (createBenchmarkMeterProvider, BenchmarkMetricSnapshot, BenchmarkScopeMetrics, BenchmarkMetric, BenchmarkDataPoint, collectBenchmarkMetrics, handleBenchmarkStats, handleBenchmarkTrigger, and BenchmarkTriggerOptions) describing their purpose, parameter types, return types, and any important semantics (e.g., snapshot shape, units, and error behavior) so they conform to the project's /**/src/**/*.ts documentation guideline; place the comments immediately above each declaration and mirror the style used elsewhere in the federation surface (brief description, `@param/`@returns where applicable).
146-154:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReturn a queue-correlation handle from
/trigger.The response only includes
activityIdand counts, so the benchmark controller still has no stable handle to correlate a trigger burst with later queue drain. That misses one of this feature's stated goals, and the root cause is that this path delegates toContext.sendActivity()without surfacing any enqueue/batch identifier back out.🤖 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 `@packages/fedify/src/federation/bench.ts` around lines 146 - 154, The response from the /trigger handler currently returns only activityId and counts and must also return a stable queue-correlation handle; update the flow around Context.sendActivity in packages/fedify/src/federation/bench.ts so you capture the enqueue/batch identifier returned by context.sendActivity (or change Context.sendActivity to return one if it doesn't yet) and include that identifier (e.g., queueCorrelationId or batchId) in the jsonResponse payload alongside activityId, using a nullable fallback if absent; ensure the symbol names referenced are context.sendActivity and jsonResponse and that the new field is returned with the 202 response.packages/fedify/src/federation/metrics.ts (1)
100-103: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSDoc for the exported interface.
QueueDepthGaugeEntryis exported and used by the publicregisterQueueDepthGauge()API, so it should have a JSDoc comment explaining its purpose. As per coding guidelines, include JSDoc comments for public APIs in**/src/**/*.tsfiles.📝 Proposed JSDoc addition
+/** + * A queue to observe for `fedify.queue.depth`. + * `@since` 2.3.0 + */ export interface QueueDepthGaugeEntry { role: QueueTaskRole; queue?: MessageQueue; }🤖 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 `@packages/fedify/src/federation/metrics.ts` around lines 100 - 103, Add a JSDoc block above the exported QueueDepthGaugeEntry interface describing that it represents an entry for queue depth metrics used by the public registerQueueDepthGauge() API; document the role property as the QueueTaskRole indicating which task/type the gauge tracks and the optional queue property as the MessageQueue instance (or undefined when no specific queue is associated), and mark the interface as part of the public API so consumers understand its purpose and fields.
🤖 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.
Outside diff comments:
In `@packages/fedify/src/federation/bench.ts`:
- Around line 28-87: Public exports in this file lack JSDoc; add short JSDoc
comments for each exported symbol (createBenchmarkMeterProvider,
BenchmarkMetricSnapshot, BenchmarkScopeMetrics, BenchmarkMetric,
BenchmarkDataPoint, collectBenchmarkMetrics, handleBenchmarkStats,
handleBenchmarkTrigger, and BenchmarkTriggerOptions) describing their purpose,
parameter types, return types, and any important semantics (e.g., snapshot
shape, units, and error behavior) so they conform to the project's
/**/src/**/*.ts documentation guideline; place the comments immediately above
each declaration and mirror the style used elsewhere in the federation surface
(brief description, `@param/`@returns where applicable).
- Around line 146-154: The response from the /trigger handler currently returns
only activityId and counts and must also return a stable queue-correlation
handle; update the flow around Context.sendActivity in
packages/fedify/src/federation/bench.ts so you capture the enqueue/batch
identifier returned by context.sendActivity (or change Context.sendActivity to
return one if it doesn't yet) and include that identifier (e.g.,
queueCorrelationId or batchId) in the jsonResponse payload alongside activityId,
using a nullable fallback if absent; ensure the symbol names referenced are
context.sendActivity and jsonResponse and that the new field is returned with
the 202 response.
In `@packages/fedify/src/federation/metrics.ts`:
- Around line 100-103: Add a JSDoc block above the exported QueueDepthGaugeEntry
interface describing that it represents an entry for queue depth metrics used by
the public registerQueueDepthGauge() API; document the role property as the
QueueTaskRole indicating which task/type the gauge tracks and the optional queue
property as the MessageQueue instance (or undefined when no specific queue is
associated), and mark the interface as part of the public API so consumers
understand its purpose and fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6c06dcfa-7d36-457b-9a6b-bde156f35130
📒 Files selected for processing (9)
CHANGES.mddocs/manual/benchmarking.mdpackages/fedify/src/federation/bench.tspackages/fedify/src/federation/builder.tspackages/fedify/src/federation/federation.tspackages/fedify/src/federation/metrics.test.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.test.tspackages/fedify/src/federation/middleware.ts
Add JSDoc for the benchmark metric snapshot and endpoint helpers so the new public exports describe their response shapes and collection behavior. Also make the trigger response explicitly include a nullable queue correlation field, matching the current Context.sendActivity() API which does not return a queue or batch identifier. Assisted-by: Codex:gpt-5.5
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 968d687a48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
There was a problem hiding this comment.
Code Review
This pull request introduces a cooperative benchmarking mode (benchmarkMode) to Fedify, enabling local benchmark endpoints under /.well-known/fedify/bench/ for stats collection and activity triggering. It configures an in-process OpenTelemetry metrics reader, reports queue depth via the new fedify.queue.depth gauge, and adjusts default behaviors—such as disabling signature time windows and allowing private addresses—to facilitate testing. Comprehensive documentation, unit tests, and metric registrations have been added to support this feature. As there are no review comments provided, I have no feedback to offer on the review itself.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/manual/benchmarking.md (1)
166-168:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse reference-style link syntax in this section.
This section uses an inline link, but the markdown rule for this PR requires reference-style links placed at the end of the section.
Suggested change
-Benchmark mode uses the same Fedify metrics documented in -[*OpenTelemetry*](./opentelemetry.md), including queue task metrics, queue +Benchmark mode uses the same Fedify metrics documented in +[*OpenTelemetry*][opentelemetry], including queue task metrics, queue depth, HTTP server metrics, and signature verification histograms. The benchmark endpoints themselves are classified as `fedify.endpoint=benchmark` in `fedify.http.server.request.*` metrics. + +[opentelemetry]: ./opentelemetry.mdAs per coding guidelines, “Use reference-style links at the end of each section.”
🤖 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/benchmarking.md` around lines 166 - 168, Replace the inline markdown link "[*OpenTelemetry*](./opentelemetry.md)" with a reference-style link: change the inline occurrence in the Benchmark mode paragraph to use a reference label (e.g. [OpenTelemetry]) and add the corresponding reference entry for ./opentelemetry.md at the end of the section; ensure the visible text matches the original ("OpenTelemetry" or "*OpenTelemetry*") and that the reference label is unique within the document.
🤖 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.
Outside diff comments:
In `@docs/manual/benchmarking.md`:
- Around line 166-168: Replace the inline markdown link
"[*OpenTelemetry*](./opentelemetry.md)" with a reference-style link: change the
inline occurrence in the Benchmark mode paragraph to use a reference label (e.g.
[OpenTelemetry]) and add the corresponding reference entry for
./opentelemetry.md at the end of the section; ensure the visible text matches
the original ("OpenTelemetry" or "*OpenTelemetry*") and that the reference label
is unique within the document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65efc499-2cd0-4b11-8b85-61fd6fb4a095
📒 Files selected for processing (4)
docs/manual/benchmarking.mdpackages/fedify/src/federation/bench.tspackages/fedify/src/federation/metrics.tspackages/fedify/src/federation/middleware.test.ts
Use the triggered activity ID as the benchmark queue correlation handle so successful trigger responses return a value that is also preserved on queued fanout and outbox work. fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cooperative benchmarkMode to Fedify, enabling local benchmark endpoints (/.well-known/fedify/bench/stats and /.well-known/fedify/bench/trigger) and in-process metric collection. It also adds a new fedify.queue.depth gauge metric to report queue depths. Feedback on the changes suggests adding a unique identifier (such as a federation instance ID or origin) to the queue depth metric attributes to prevent conflicts and overwritten values when multiple Federation instances share the same meterProvider in a single process.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42a7c52a05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Give each Federation instance an opaque queue depth source ID so two instances sharing one MeterProvider do not emit identical gauge attribute sets. Register queue depth callbacks again when the effective global MeterProvider changes, matching the fallback used by other metrics. fedify-dev#787 (comment) fedify-dev#787 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cooperative benchmarkMode to Fedify, enabling local benchmarking of federation workloads. When enabled, it exposes GET /.well-known/fedify/bench/stats for in-process OpenTelemetry metric snapshots and POST /.well-known/fedify/bench/trigger to drive activity delivery to configured sink recipients. Additionally, it registers a new fedify.queue.depth gauge to observe queue depths, configures explicit low-latency buckets for signature verification duration, and relaxes security defaults (such as private address checks and signature time windows) specifically for benchmark targets. Comprehensive unit tests and documentation have been added to support these changes. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ 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". |
Cloudflare Workers forbids generating random values while evaluating the global scope. The queue depth gauge only needs an opaque per-process Federation identifier to keep series distinct on a shared MeterProvider, so use a local counter instead of crypto.randomUUID(). Update the OpenTelemetry attribute example so it does not imply that this identifier is a UUID. Assisted-by: Codex:gpt-5.5
1 participant
Closes #782; part of #744.
Summary
This PR adds
benchmarkModetocreateFederation()andFederationBuilder.build()so a Fedify application can opt into cooperative benchmark behavior when it is used as a benchmark target.Benchmark mode exposes two target-side endpoints under
/.well-known/fedify/bench/: a stats endpoint that returns an in-process OpenTelemetry metric snapshot, and a trigger endpoint that drives the application's normalContext.sendActivity()path against explicit benchmark sink recipients. The trigger endpoint rejects recipients outside the sink list by default, with an explicit override for controlled runs.The mode also applies benchmark-target defaults that make local benchmark runs practical:
allowPrivateAddressdefaults totruewhen the built-in document loaders are used, andsignatureTimeWindowdefaults tofalse. Explicit option values still win, custom document loader factories keep the existing private-address behavior, and inbox idempotency is unchanged.What changed
FederationOptions.benchmarkModeand threaded it throughcreateFederation()andFederationBuilder.build().fedify.queue.depthas a regular Fedify OpenTelemetry gauge for queues that implementMessageQueue.getDepth(), including non-benchmark applications with a configuredmeterProvider.activitypub.signature.verification.duration.Meter provider behavior
Benchmark mode owns its own
MeterProviderbecause the benchmark stats endpoint needs a reader attached when the provider is constructed. If an application passesbenchmarkMode: trueand a custommeterProviderat the same time, Fedify throws aTypeError.The docs include the intended pattern for apps that use one code path for normal runs and benchmark runs:
Verification
I ran the following checks locally:
deno test -A packages/fedify/src/federation/middleware.test.ts --filter "benchmarkMode"deno test -A packages/fedify/src/federation/middleware.test.ts --filter "createFederation() registers queue depth for regular metrics"deno test -A packages/fedify/src/federation/metrics.test.ts --filter "signature verification duration uses explicit low-latency buckets"mise run checkpnpm --dir docs buildI also confirmed the queue depth regression test failed before the gauge registration was moved out of the benchmark-only branch.