Handle large log messages more efficiently #1604
Conversation
…n the summary tree view
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/webapp/app/v3/eventRepository.server.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces changes to two server-side TypeScript files in the webapp application. In Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Nitpick comments (2)
apps/webapp/app/v3/eventRepository.server.ts (1)
181-198: Refactor to eliminate duplication of selected fieldsThe list of fields in
TaskEventSummaryand the raw SQL query ingetTraceSummaryare identical. Maintaining identical field lists in multiple places can lead to inconsistencies if one is updated without the other. To adhere to the DRY principle and improve maintainability, consider defining the field list in a shared constant or utility function that can be reused in both the type definition and the SQL query.Proposed change:
Define a constant for the fields:
const TASK_EVENT_FIELDS = [ "id", "spanId", "parentId", "runId", "idempotencyKey", "message", "style", "startTime", "duration", "isError", "isPartial", "isCancelled", "level", "events", "environmentType", ] as const;Use it in the type definition:
type TaskEventSummary = Pick<TaskEvent, (typeof TASK_EVENT_FIELDS)[number]>;And update the SQL query:
const events = await this.readReplica.$queryRaw<TaskEventSummary[]>` SELECT ${Prisma.join( TASK_EVENT_FIELDS.map((field) => Prisma.sql`"${Prisma.raw(field)}"`) )} FROM "TaskEvent" WHERE "traceId" = ${traceId} ORDER BY "startTime" ASC LIMIT ${env.MAXIMUM_TRACE_SUMMARY_VIEW_COUNT} `;apps/webapp/app/v3/otlpExporter.server.ts (1)
170-172: Define a constant for maximum message lengthThe hardcoded value
4096used for truncating themessagemakes future adjustments harder and reduces readability. Consider defining a constant for the maximum message length to improve maintainability and readability.Additional code (outside selected lines):
const MAX_MESSAGE_LENGTH = 4096;Updated code:
-message: isStringValue(log.body) - ? log.body.stringValue.slice(0, 4096) +message: isStringValue(log.body) + ? log.body.stringValue.slice(0, MAX_MESSAGE_LENGTH) : `${log.severityText} log`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/v3/eventRepository.server.ts(2 hunks)apps/webapp/app/v3/otlpExporter.server.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/v3/eventRepository.server.ts (2)
405-426: Verify parameterization of variables in raw SQL queryWhile Prisma's
$queryRawwith tagged template literals generally handles parameterization safely, it's important to ensure that variables like${traceId}and${env.MAXIMUM_TRACE_SUMMARY_VIEW_COUNT}are correctly parameterized to prevent SQL injection attacks.
412-412: Check consistency in message truncation lengthIn the SQL query,
LEFT(message, 256) as messagetruncates themessagefield to 256 characters. However, inotlpExporter.server.ts, themessageis truncated to 4096 characters. Ensure that the truncation length is consistent across the application or adjust appropriately based on requirements.
| message: isStringValue(log.body) | ||
| ? log.body.stringValue.slice(0, 4096) | ||
| : `${log.severityText} log`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential issues with multibyte characters when truncating
Using slice(0, 4096) on a string may cut through multibyte characters, potentially leading to malformed text. To properly handle multibyte characters, consider using methods that are Unicode-aware, such as substring or libraries that handle Unicode strings correctly.
Proposed change:
message: isStringValue(log.body)
? log.body.stringValue.substring(0, MAX_MESSAGE_LENGTH)
: `${log.severityText} log`,Alternatively, if the limit is intended to be in bytes, use a library that accurately handles UTF-8 strings.
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/sdk
commit: |
Summary by CodeRabbit
New Features
TaskEventSummaryfor more precise event trackingImprovements
Performance