Added testing for tracing #1068
Conversation
WalkthroughA new asynchronous GET endpoint at The The OpenTelemetry and Sentry instrumentation setup in The CI/CD workflow Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
allouis
force-pushed
the
negative-post-count-error
branch
from
f18a12d to
2bb0b99
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/app.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: allouis
PR: TryGhost/ActivityPub#593
File: src/http/api/reply.ts:48-59
Timestamp: 2025-04-30T11:39:20.993Z
Learning: In the TryGhost/ActivityPub repository, when working with URL IDs in API handlers, the codebase uses a pattern where the raw ID parameter is passed to `lookupObject` even when a decoded version has been created with `decodeURIComponent`. This pattern is consistent with existing functionality in `handlers.ts`.
Learnt from: allouis
PR: TryGhost/ActivityPub#1021
File: src/post/post.entity.ts:327-329
Timestamp: 2025-07-10T08:39:14.900Z
Learning: In the TryGhost/ActivityPub repository, when parsing data from Ghost CMS (such as post excerpts) using branded types with validation (like PostSummary.parse()), allouis prefers to let the parsing throw errors if Ghost sends incompatible data rather than adding defensive handling like truncation. This approach helps detect incompatibilities between the ActivityPub system and Ghost CMS early, ensuring issues are addressed at the source rather than silently ignored.
Learnt from: vershwal
PR: TryGhost/ActivityPub#1033
File: src/http/api/webhook.controller.ts:103-107
Timestamp: 2025-07-14T11:41:50.880Z
Learning: In src/http/api/webhook.controller.ts, the handlePostUnpublished, handlePostUpdated, and handlePostDeleted methods in PR #1033 are intentionally implemented as dummy APIs that return 200 status with null body. These are placeholder implementations to prevent URL errors while the actual backend logic will be built in upcoming PRs, following the incremental development approach used in this codebase.
Learnt from: mike182uk
PR: TryGhost/ActivityPub#1043
File: src/account/account.service.ts:759-794
Timestamp: 2025-07-15T13:07:04.697Z
Learning: In the TryGhost/ActivityPub repository, the exponential backoff for delivery failures in src/account/account.service.ts is intentionally designed without a maximum cap. This unbounded growth is by design - longer delivery failure periods give more confidence that an account is truly broken rather than temporarily unavailable, while still giving receivers a fair chance to recover from temporary issues.
Learnt from: mike182uk
PR: TryGhost/ActivityPub#1043
File: src/account/account.service.ts:54-55
Timestamp: 2025-07-15T13:05:42.512Z
Learning: In the TryGhost/ActivityPub repository, mike182uk designed the delivery failure backoff mechanism in src/account/account.service.ts to intentionally use unbounded exponential backoff without a maximum cap. This design serves as a confidence mechanism where the longer delivery attempts fail, the more confident the system becomes that the account is truly broken rather than experiencing temporary issues. The backoff gives receivers a "fair chance" of receiving future activities if they're having temporary issues that later get resolved.
Learnt from: allouis
PR: TryGhost/ActivityPub#858
File: features/support/content.js:15-22
Timestamp: 2025-06-16T15:43:23.967Z
Learning: allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. Functional improvements like error handling should be suggested for separate PRs.
| @@ -19,7 +19,7 @@ | |||
| "migration": "docker compose run migrate create", | |||
| "migrate": "docker compose up migrate", | |||
| "test:cucumber": "./dev/docker/cucumber-tests", | |||
| "test": "docker compose run --rm migrate-testing up && docker compose run --rm -v \"$(pwd)/dist:/opt/activitypub/dist\" activitypub-testing yarn _test:all && yarn test:cucumber", | |||
| "test": "echo 'skip tests'", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Disabling the test script silently removes a key safety net
"test": "echo 'skip tests'" prevents any unit/integration/type tests from running locally or in CI, masking regressions and security issues.
If the intent is only to speed up development, prefer pointing test at the existing aggregated script (test:all) or gating the skip behind an env-flag so CI still exercises the suite.
- "test": "echo 'skip tests'",
+ "test": "yarn test:all",If you truly need to skip tests in some contexts, add a dedicated "test:skip" script and leave the canonical test hook intact.
To verify whether there’s an existing test:all (or similar) script to point the canonical test at, let’s inspect the scripts section:
🏁 Script executed:
#!/bin/bash
# List all scripts defined in package.json
echo "⮕ Scripts in package.json:"
jq '.scripts' package.json || true
# Check explicitly for test:all
echo
echo "⮕ Looking for a test:all script:"
rg -n '"test:all"' package.json || echo "No test:all script found"Length of output: 2365
Restore the canonical test script to run the full suite
The repo already defines a test:all script that runs types and code checks inside Docker. Overriding test to echo 'skip tests' silences all tests and risks undetected regressions. Update test to point at test:all, and if you need skips, introduce a separate test:skip hook.
• File: package.json (around line 22)
• Replace the existing "test" line with:
- "test": "echo 'skip tests'",
+ "test": "yarn test:all",• (Optional) Add a dedicated skip script:
+ "test:skip": "echo 'skip tests'",📝 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.
| "test": "echo 'skip tests'", | |
| "test": "yarn test:all", | |
| "test:skip": "echo 'skip tests'", |
🤖 Prompt for AI Agents
In package.json around line 22, the current "test" script is set to "echo 'skip
tests'", which disables all tests and risks missing regressions. Change the
"test" script to run the full test suite by setting it to "npm run test:all".
Optionally, add a new script "test:skip" with the value "echo 'skip tests'" if
you want a dedicated command to skip tests without affecting the main test
script.
f0c9476 to
b276efb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/instrumentation.ts (2)
117-125: Consider refactoring duplicate Google Cloud Trace exporter setup.The Google Cloud Trace exporter setup is duplicated between Sentry (lines 84-91) and OpenTelemetry configurations. Consider extracting the exporter creation into a shared function to improve maintainability.
+async function createGoogleCloudTraceExporter() { + const { TraceExporter } = await import( + '@google-cloud/opentelemetry-cloud-trace-exporter' + ); + return new TraceExporter({}); +} // In Sentry setup (line 88-90) - const { TraceExporter } = await import( - '@google-cloud/opentelemetry-cloud-trace-exporter' - ); sentryClient?.traceProvider?.addSpanProcessor( - new BatchSpanProcessor(new TraceExporter({})), + new BatchSpanProcessor(await createGoogleCloudTraceExporter()), ); // In OpenTelemetry setup (line 119-124) - const { TraceExporter } = await import( - '@google-cloud/opentelemetry-cloud-trace-exporter' - ); provider.addSpanProcessor( - new BatchSpanProcessor(new TraceExporter({})), + new BatchSpanProcessor(await createGoogleCloudTraceExporter()), );
127-139: Consider refactoring duplicate OTLP exporter setup.Similar to the Google Cloud Trace exporter, the OTLP exporter setup is duplicated between Sentry (lines 93-104) and OpenTelemetry configurations.
+async function createOTLPTraceExporter() { + const { OTLPTraceExporter } = await import( + '@opentelemetry/exporter-trace-otlp-proto' + ); + return new OTLPTraceExporter({ + url: 'http://jaeger:4318/v1/traces', + }); +} // In Sentry setup (line 94-103) - const { OTLPTraceExporter } = await import( - '@opentelemetry/exporter-trace-otlp-proto' - ); sentryClient?.traceProvider?.addSpanProcessor( new SimpleSpanProcessor( - new OTLPTraceExporter({ - url: 'http://jaeger:4318/v1/traces', - }), + await createOTLPTraceExporter(), ), ); // In OpenTelemetry setup (line 129-138) - const { OTLPTraceExporter } = await import( - '@opentelemetry/exporter-trace-otlp-proto' - ); provider.addSpanProcessor( new SimpleSpanProcessor( - new OTLPTraceExporter({ - url: 'http://jaeger:4318/v1/traces', - }), + await createOTLPTraceExporter(), ), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app.ts(2 hunks)src/instrumentation.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: allouis
PR: TryGhost/ActivityPub#593
File: src/http/api/reply.ts:48-59
Timestamp: 2025-04-30T11:39:20.993Z
Learning: In the TryGhost/ActivityPub repository, when working with URL IDs in API handlers, the codebase uses a pattern where the raw ID parameter is passed to `lookupObject` even when a decoded version has been created with `decodeURIComponent`. This pattern is consistent with existing functionality in `handlers.ts`.
Learnt from: mike182uk
PR: TryGhost/ActivityPub#1043
File: src/account/account.service.ts:759-794
Timestamp: 2025-07-15T13:07:04.697Z
Learning: In the TryGhost/ActivityPub repository, the exponential backoff for delivery failures in src/account/account.service.ts is intentionally designed without a maximum cap. This unbounded growth is by design - longer delivery failure periods give more confidence that an account is truly broken rather than temporarily unavailable, while still giving receivers a fair chance to recover from temporary issues.
Learnt from: allouis
PR: TryGhost/ActivityPub#1021
File: src/post/post.entity.ts:327-329
Timestamp: 2025-07-10T08:39:14.900Z
Learning: In the TryGhost/ActivityPub repository, when parsing data from Ghost CMS (such as post excerpts) using branded types with validation (like PostSummary.parse()), allouis prefers to let the parsing throw errors if Ghost sends incompatible data rather than adding defensive handling like truncation. This approach helps detect incompatibilities between the ActivityPub system and Ghost CMS early, ensuring issues are addressed at the source rather than silently ignored.
Learnt from: mike182uk
PR: TryGhost/ActivityPub#1043
File: src/account/account.service.ts:54-55
Timestamp: 2025-07-15T13:05:42.512Z
Learning: In the TryGhost/ActivityPub repository, mike182uk designed the delivery failure backoff mechanism in src/account/account.service.ts to intentionally use unbounded exponential backoff without a maximum cap. This design serves as a confidence mechanism where the longer delivery attempts fail, the more confident the system becomes that the account is truly broken rather than experiencing temporary issues. The backoff gives receivers a "fair chance" of receiving future activities if they're having temporary issues that later get resolved.
Learnt from: vershwal
PR: TryGhost/ActivityPub#1033
File: src/http/api/webhook.controller.ts:103-107
Timestamp: 2025-07-14T11:41:50.880Z
Learning: In src/http/api/webhook.controller.ts, the handlePostUnpublished, handlePostUpdated, and handlePostDeleted methods in PR #1033 are intentionally implemented as dummy APIs that return 200 status with null body. These are placeholder implementations to prevent URL errors while the actual backend logic will be built in upcoming PRs, following the incremental development approach used in this codebase.
Learnt from: allouis
PR: TryGhost/ActivityPub#858
File: features/support/content.js:15-22
Timestamp: 2025-06-16T15:43:23.967Z
Learning: allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. Functional improvements like error handling should be suggested for separate PRs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (2)
src/instrumentation.ts (2)
2-14: LGTM! Comprehensive OpenTelemetry imports.The import statements correctly include all necessary OpenTelemetry modules for tracing setup, and the switch to
sdk-trace-nodeis appropriate for Node.js applications.
33-40: Missing global provider registration for OpenTelemetry.The
NodeTracerProvideris created and configured but never registered globally. Without callingprovider.register()after all span processors are added, the OpenTelemetry instrumentation won't work as expected.Add this after all span processors are configured (after line 139):
+ // Register the provider globally + provider.register();Likely an incorrect or invalid review comment.
src/instrumentation.ts
Outdated
| export function otelSpanWrapper<TArgs extends unknown[], TReturn>( | ||
| fn: (...args: TArgs) => TReturn, | ||
| ) { | ||
| const tracer = trace.getTracer('activitypub', '1.0.0'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent service version for tracer.
The tracer is using a hardcoded version '1.0.0' while the Resource configuration uses process.env.K_REVISION || '1.0.0'. Consider using the same version for consistency.
- const tracer = trace.getTracer('activitypub', '1.0.0');
+ const tracer = trace.getTracer('activitypub', process.env.K_REVISION || '1.0.0');📝 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.
| const tracer = trace.getTracer('activitypub', '1.0.0'); | |
| const tracer = trace.getTracer('activitypub', process.env.K_REVISION || '1.0.0'); |
🤖 Prompt for AI Agents
In src/instrumentation.ts at line 159, the tracer version is hardcoded as
'1.0.0' which is inconsistent with the Resource configuration that uses
process.env.K_REVISION || '1.0.0'. Update the tracer initialization to use the
same version value by replacing the hardcoded string with process.env.K_REVISION
|| '1.0.0' to maintain consistency.
No description provided.