Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSentry is integrated broadly: package scripts load a Sentry setup module, Sentry packages are upgraded, Sentry is initialised at module load using dotenv, controllers and services emit Sentry metrics on success/failure, and the logger adds a Winston transport that forwards warn/error logs to Sentry. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…s-and-logging-allowing-initial-dashboard-creation
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/utils/logger.js (1)
13-25: Prefer Sentry's Winston transport helper here.The SDK already ships
createSentryWinstonTransport(Transport), so carrying a bespoke transport just adds maintenance surface for a first-party integration. Winston's own transport guidance also treatswinston-transportas the base contract for custom transports. (github.com)♻️ Proposed refactor
-class SentryTransport extends Transport { - log (info, callback) { - if (info.level === 'warn') Sentry.logger.warn(info.message, info) - else if (info.level === 'error') Sentry.logger.error(info.message, info) - callback() - } -} +const SentryTransport = Sentry.createSentryWinstonTransport(Transport)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger.js` around lines 13 - 25, Replace the custom SentryTransport with Sentry's provided helper: remove the SentryTransport class and use createSentryWinstonTransport (or createSentryWinstonTransport(Transport) from Sentry SDK) when building appTransports so the Sentry transport is instantiated and configured by the SDK; update appTransports to include new createSentryWinstonTransport({ level: 'warn' }) (or equivalent factory call) alongside transports.Console and transports.File and ensure any options previously passed into SentryTransport (e.g., level) are forwarded to the SDK factory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controllers/resultsController.js`:
- Around line 82-88: The current branch emits the raw errMsg to Sentry metrics
and only sets Sentry.getCurrentScope().setTag('async_handled_processing_error',
true) in the non-empty branch; instead normalize errMsg into a bounded failure
category (e.g., map error text to a fixed code or type) when calling
Sentry.metrics.count('url_submission.async_processing_failure', 1, { attributes:
{ error_category: ... } }) and ensure suppression is applied consistently by
either setting the 'async_handled_processing_error' tag in both branches or,
better, mark the created MiddlewareError instance itself (e.g., a
handledAsyncProcessing flag/property passed in the third-argument metadata) so
suppression logic can read that marker rather than mutating getCurrentScope().
In `@src/controllers/submitUrlController.js`:
- Around line 42-43: The logs in submitUrlController.js currently send the raw
user-supplied submittedUrl to external sinks (via logger.warn/logger.error and
Sentry), so replace the raw URL with a scrubbed value before logging: add or use
a helper (e.g., redactUrl(url) or hashUrl(url) in utils) that either strips
query strings/sensitive fragments or returns a stable hash/origin+path, then
update the logger.warn/logger.error calls (and any Sentry-related log
attributes) in the submit handler (the submit URL controller where logger.warn
is called) and at the other noted call sites (lines referenced 64-65, 79-80,
86-87, 93-94) to pass the scrubbed value instead of submittedUrl; ensure the
helper is deterministic and does not retain query parameters or sensitive
tokens.
- Around line 91-95: postCheckFailure branch logs omit the funnel event field;
update the failure logging so the logger.warn call in the postCheckFailure
handling includes event: 'url_submission_failure' (the Sentry.metrics.count line
can remain unchanged). Locate the postCheckFailure =
postValidators(headResponse).find(...) and add the event property to the
logger.warn payload (preserve existing keys failure_type, type, submittedUrl) so
exists/restricted403/size failures are recorded under the expected event key.
In `@src/services/datasette.js`:
- Around line 29-30: The metric and log currently use the full Datasette URL
(variable url) which contains encoded SQL and creates high-cardinality metric
labels; update Sentry.metrics.count call to remove url from attributes and
instead use stable dimensions such as database and a short query category/name
(e.g. derive a small queryName from the query or pass queryCategory) and modify
logger.warn (in the runQuery()/runQuery function context) to avoid embedding the
full url in metric attributes—keep url only in the warning message or
strip/obfuscate SQL if you must log it, and ensure
Sentry.metrics.count('datasette_query_errors', 1, { attributes: { database,
queryName } }) is used instead of including url.
In `@src/services/platformApi.js`:
- Line 147: The Sentry.metrics.count call currently tags the counter with the
full request URL (Sentry.metrics.count('platform_api_errors', 1, { attributes: {
url } })), which creates unbounded time series; change it to use bounded
attributes instead (e.g., derive and pass endpoint name/path template, HTTP
status family like 5xx/4xx, and a short failure reason code) so the metric uses
fixed cardinality labels; update the call site in platformApi.js to replace the
url attribute with those bounded attributes (endpoint, statusFamily,
failureReason) while keeping the metric name the same.
---
Nitpick comments:
In `@src/utils/logger.js`:
- Around line 13-25: Replace the custom SentryTransport with Sentry's provided
helper: remove the SentryTransport class and use createSentryWinstonTransport
(or createSentryWinstonTransport(Transport) from Sentry SDK) when building
appTransports so the Sentry transport is instantiated and configured by the SDK;
update appTransports to include new createSentryWinstonTransport({ level: 'warn'
}) (or equivalent factory call) alongside transports.Console and transports.File
and ensure any options previously passed into SentryTransport (e.g., level) are
forwarded to the SDK factory.
🪄 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: CHILL
Plan: Pro
Run ID: 29eb08f7-f21d-4289-9769-527577c0b1e5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.jsonsrc/controllers/resultsController.jssrc/controllers/submitUrlController.jssrc/serverSetup/sentry.jssrc/services/datasette.jssrc/services/platformApi.jssrc/utils/logger.js
…ng-allowing-initial-dashboard-creation' of https://github.com/digital-land/submit into 2432-initial-update-to-provide-to-use-metrics-and-logging-allowing-initial-dashboard-creation
pooleycodes
deleted the
2432-initial-update-to-provide-to-use-metrics-and-logging-allowing-initial-dashboard-creation
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Sentry improvements
Packages
@sentry/node+@sentry/profiling-nodeupgraded to v10.47.0Sentry init (
sentry.js)Sentry.init()to module top-level + added--importto all start scripts inpackage.json(ESM instrumentation requirement)enableLogs: truebeforeSendto suppressasync_handled_processing_errortagged events from becoming issuesWinston transport (
logger.js)SentryTransportforwardingwarn/errorlogs to Sentry log ingestionURL submission metrics (
submitUrlController.js)url_submission.begunurl_submission.validation_failurefailure_typeattribute)url_submission.head_request_errorreasonattribute)url_submission.async_request_failureurl_submission.acceptedlogger.warnwithevent: url_submission_failure+submittedUrlfor Sentry Logs widgetResults metrics (
resultsController.js)url_submission.successurl_submission.async_processing_failureerror_messageattribute)Dashboard funnel
begun → accepted → successSuccess rate =
success / begun(formula widget)What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
If possible run in local, using a local sentry env set in .env file, then run through some test submissions to see outputs in the various dashboards created for the provide service and can see the logging pass through.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
Summary by CodeRabbit