Disabled logging event payload on event success #499
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
WalkthroughThe handler now extracts a local page-hit payload via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (4)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/events/publisherUtils.ts (1)
7-13: Critical: Unreachable code due to throw statement - publishing is disabled.The
throw new Error('Publishing page hit raw events is disabled')at line 8 makes thepublishEventcall at lines 9-13 unreachable dead code. Based on the commit message "Added temporary error throw to test on staging", this appears to be debug code that should be removed before merging.With this code in place, all page hit raw event publishing will fail when
PUBSUB_TOPIC_PAGE_HITS_RAWis configured.🔎 Remove the temporary throw statement
export const publishPageHitRaw = async (request: PageHitRequestType, payload: PageHitRaw): Promise<void> => { const topic = process.env.PUBSUB_TOPIC_PAGE_HITS_RAW as string; if (topic) { request.log.info({event_id: payload.payload.event_id}, 'Publishing page hit raw event'); - throw new Error('Publishing page hit raw events is disabled'); await publishEvent({ topic, payload, logger: request.log }); } };
🤖 Fix all issues with AI Agents
In @test/unit/services/events/publisherUtils.test.ts:
- Around line 64-70: The test name in
test/unit/services/events/publisherUtils.test.ts is misleading: rename the
"should call publishEvent with correct parameters" test to reflect that
publishing is disabled (e.g., "should not call publishEvent when publishing is
disabled") so it matches the assertions that publishPageHitRaw(mockRequest,
mockPayload) rejects with 'Publishing page hit raw events is disabled' and that
the publishEvent spy (publishEventSpy on publisherModule.publishEvent) was not
called; update only the test description string to keep assertions and spy usage
unchanged.
🧹 Nitpick comments (1)
test/unit/services/events/publisherUtils.test.ts (1)
34-54: Test names are misleading given the temporary disabled state.The test is named "should log info message with only event_id on successful publish" but actually expects a rejection with "Publishing page hit raw events is disabled". While this tests the current behavior correctly, the test name doesn't reflect reality.
Consider renaming to clarify the temporary state, e.g., "should log info message with only event_id before throwing disabled error".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/handlers/page-hit-handlers.tssrc/services/events/publisherUtils.tstest/unit/handlers/page-hit-handlers.test.tstest/unit/services/events/publisherUtils.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/unit/services/events/publisherUtils.test.ts (3)
src/schemas/v1/page-hit-request.ts (1)
PageHitRequestType(111-115)src/schemas/v1/page-hit-raw.ts (1)
PageHitRaw(63-63)src/services/events/publisherUtils.ts (1)
publishPageHitRaw(4-15)
test/unit/handlers/page-hit-handlers.test.ts (1)
src/handlers/page-hit-handlers.ts (1)
handlePageHitRequestStrategyBatch(6-32)
src/services/events/publisherUtils.ts (2)
src/schemas/v1/page-hit-request.ts (1)
PageHitRequestType(111-115)src/schemas/v1/page-hit-raw.ts (1)
PageHitRaw(63-63)
src/handlers/page-hit-handlers.ts (2)
src/transformations/page-hit-transformations.ts (1)
pageHitRawPayloadFromRequest(4-48)src/services/events/publisherUtils.ts (1)
publishPageHitRaw(4-15)
⏰ 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). (3)
- GitHub Check: Lint and Test (Node 22)
- GitHub Check: Lint and Test (Node 20)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
test/unit/handlers/page-hit-handlers.test.ts (2)
86-96: LGTM! Test correctly verifies the updated function signature and payload flow.The test properly mocks
pageHitRawPayloadFromRequest, verifies the two-argument call topublishPageHitRaw, and ensures the success path works as expected.
98-250: Comprehensive error scenario coverage with payload logging.The error tests correctly verify that:
publishPageHitRawis called with bothrequestandmockPayload- The
payloadfield is included in error logs for debugging- Various error types (Error, string, null, undefined, complex objects) are handled
This aligns well with the PR objective of logging payloads only on failure.
test/unit/services/events/publisherUtils.test.ts (1)
72-80: LGTM! Correctly tests the no-op path when environment variable is unset.This test properly verifies that when
PUBSUB_TOPIC_PAGE_HITS_RAWis not configured, the function exits early without logging or attempting to publish.src/handlers/page-hit-handlers.ts (1)
6-31: Well-structured refactor that addresses the previous review concern.Moving
pageHitRawPayloadFromRequest(request)outside the try block (line 7) eliminates the potential for calling the transformation twice on error. The payload is now:
- Extracted once before the try block
- Passed to
publishPageHitRaw- Included in error logs only on failure (aligning with PR objective to reduce logged data)
If
pageHitRawPayloadFromRequestthrows, the error will propagate to the outer handler's catch block at line 113, which provides appropriate error handling.src/services/events/publisherUtils.ts (1)
4-7: Logging improvement aligns with PR objectives.The change to log only
{event_id: payload.payload.event_id}on publish success reduces stored data while maintaining traceability. Full payload details are still captured in the handler's error logging path when publishing fails.
JoeeGrigg
force-pushed
the
log-payload-less
branch
from
53e8976 to
4c59555
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/services/events/publisherUtils.ts:
- Around line 8-10: Remove the unconditional staging-only throw that disables
publishing: delete the throw new Error('Publishing page hit raw events is
disabled') inside the if (process.env.NODE_ENV === 'staging') branch in
publisherUtils (or replace it with a non-failing behavior such as a debug log or
a feature-flag check). Ensure event publishing is not blocked in staging by
either removing the throw entirely or gating it behind a proper config flag
before merging.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/events/publisherUtils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/events/publisherUtils.ts (2)
src/schemas/v1/page-hit-request.ts (1)
PageHitRequestType(111-115)src/schemas/v1/page-hit-raw.ts (1)
PageHitRaw(63-63)
⏰ 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). (3)
- GitHub Check: Lint and Test (Node 20)
- GitHub Check: Lint and Test (Node 22)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
src/services/events/publisherUtils.ts (3)
1-2: LGTM on import changes.The import correctly brings in the
PageHitRawtype to match the new function signature.
4-7: Function signature and minimal logging look correct.The signature update to accept an explicit
payload: PageHitRawparameter aligns with the PR goal, and logging onlyevent_idreduces storage costs as intended.
11-15: LGTM on publishEvent invocation.The call correctly passes the topic, payload, and logger.
| if (process.env.NODE_ENV === 'staging') { | ||
| throw new Error('Publishing page hit raw events is disabled'); | ||
| } |
There was a problem hiding this comment.
Remove the staging-only throw before merging.
This unconditional throw in staging completely disables event publishing. The PR description mentions it was "a temporary error throw used for staging tests," but it must be removed before merging to main—otherwise, publishing will remain broken in staging indefinitely.
🔎 Proposed fix
request.log.info({event_id: payload.payload.event_id}, 'Publishing page hit raw event');
- if (process.env.NODE_ENV === 'staging') {
- throw new Error('Publishing page hit raw events is disabled');
- }
await publishEvent({📝 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 (process.env.NODE_ENV === 'staging') { | |
| throw new Error('Publishing page hit raw events is disabled'); | |
| } | |
| request.log.info({event_id: payload.payload.event_id}, 'Publishing page hit raw event'); | |
| await publishEvent({ |
🤖 Prompt for AI Agents
In @src/services/events/publisherUtils.ts around lines 8 - 10, Remove the
unconditional staging-only throw that disables publishing: delete the throw new
Error('Publishing page hit raw events is disabled') inside the if
(process.env.NODE_ENV === 'staging') branch in publisherUtils (or replace it
with a non-failing behavior such as a debug log or a feature-flag check). Ensure
event publishing is not blocked in staging by either removing the throw entirely
or gating it behind a proper config flag before merging.
|
✅ Deployed to staging (tree: deb9a3550863b73d12358f69d5d24019a5bb0643) |
JoeeGrigg
force-pushed
the
log-payload-less
branch
from
4c59555 to
f109602
Compare
|
✅ Deployed to staging (tree: dec09b9b5742e11e4c7302441f4f1fd2effbecec) |
cmraible
left a comment
cmraible
left a comment
There was a problem hiding this comment.
Nice one! Overall I'm definitely onboard with this — one small request (see comment)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/events/publisherUtils.ts (1)
6-14: Consider logging when publishing is disabled.The conditional publishing logic correctly implements the PR objective of allowing publishing to be disabled when
PUBSUB_TOPIC_PAGE_HITS_RAWis unset. However, the function returns silently in this case, which might make debugging configuration issues more difficult.💡 Optional: Add debug log for disabled publishing
const topic = process.env.PUBSUB_TOPIC_PAGE_HITS_RAW as string; if (topic) { request.log.info({event_id: payload.payload.event_id}, 'Publishing page hit raw event'); request.log.debug({payload}, 'Publishing page hit raw event (payload)'); await publishEvent({ topic, payload, logger: request.log }); + } else { + request.log.debug('Publishing page hit raw event skipped (PUBSUB_TOPIC_PAGE_HITS_RAW not configured)'); } };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/events/publisherUtils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/events/publisherUtils.ts (2)
src/schemas/v1/page-hit-request.ts (1)
PageHitRequestType(111-115)src/schemas/v1/page-hit-raw.ts (1)
PageHitRaw(63-63)
⏰ 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). (4)
- GitHub Check: Setup
- GitHub Check: Lint and Test (Node 20)
- GitHub Check: Lint and Test (Node 22)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
src/services/events/publisherUtils.ts (2)
1-4: LGTM! Function signature updated to accept explicit payload.The updated signature
(request: PageHitRequestType, payload: PageHitRaw)correctly addresses the read-once payload issue mentioned in the PR objectives. The corresponding import changes are appropriate, and the AI summary confirms that all callers in handlers and tests have been updated to pass the payload explicitly.
7-8: Excellent! Logging changes address PR objectives and past feedback.The minimal info logging (line 7) successfully reduces verbosity by logging only the
event_idon publish attempts, while the debug-level payload logging (line 8) addresses cmraible's past request for retained debugging capability. Additionally, the critical staging throw that previously blocked all publishing has been removed—resolving the past critical issues flagged by cursor[bot] and coderabbitai[bot].
|
❌ Staging deployment failed (tree: e7a07d9ac2ccfc237330e290e18aced292aa3b96) |
This will make it easier to debug in development or any other time debug logs are enabled. But not something that is needed all of the time in production.
JoeeGrigg
force-pushed
the
log-payload-less
branch
from
e6b7213 to
d90a6f7
Compare
|
✅ Deployed to staging (tree: 3ffff1fe97c2c57a498c3c9510abef4584b67f28) |
JoeeGrigg
dismissed
cmraible’s stale review
I've made the change but can't get GH to accept that the requested change has been made. Only dismissing to get it merged...
ref NY-363
This should save a lot of storage and cost in GCP whilst having very similar functionality available to us.
This changes our logging so that we only log the payload of a request if it fails to publish to GCP Pub/Sub. If it's successful we instead only log the event_id field and we should be able to look that up in Tinybird in the future if we ever needed it.
Note
handlePageHitRequestStrategyBatchto precomputepayloadviapageHitRawPayloadFromRequestand pass it topublishPageHitRaw; includespayloadin error logs for failurespublishPageHitRaw(request, payload)to requirePageHitRawpayload param, log only{event_id}at info level, and log fullpayloadat debug; removes internal payload constructionpublisherUtils.test.tsand updates handler tests to expect the new call signature and error logging shapeWritten by Cursor Bugbot for commit d90a6f7. This will update automatically on new commits. Configure here.