fix: Logging large objects is now much more performant and uses less memory #2263
Conversation
🦋 Changeset detectedLatest commit: 085f0af The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change increases the default limits for OpenTelemetry span and log attribute counts from 256 to 1024 across the codebase. It introduces a new class-based implementation for the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial 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). (27)
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 (
|
![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 (3)
packages/core/test/flattenAttributes.test.ts (1)
4-4: Test description could be more accurateThe test description "handles number keys correctly" doesn't fully capture what the test is verifying. The test covers both numeric string keys (like "25") and numeric keys, as well as array indexing behavior.
Consider updating the description to be more comprehensive:
- it("handles number keys correctly", () => { + it("handles numeric and numeric string keys correctly", () => {packages/core/src/v3/utils/flattenAttributes.ts (2)
167-174: Avoid usinganytype casting for TypedArraysThe
as anycast can be avoided by using proper TypeScript type guards or more specific typing.if (ArrayBuffer.isView(obj)) { - const typedArray = obj as any; - this.addAttribute(`${prefix || "typedarray"}.constructor`, typedArray.constructor.name); - this.addAttribute(`${prefix || "typedarray"}.length`, typedArray.length); - this.addAttribute(`${prefix || "typedarray"}.byteLength`, typedArray.byteLength); - this.addAttribute(`${prefix || "typedarray"}.byteOffset`, typedArray.byteOffset); + this.addAttribute(`${prefix || "typedarray"}.constructor`, obj.constructor.name); + this.addAttribute(`${prefix || "typedarray"}.length`, (obj as ArrayBufferView).byteLength / (obj.constructor as any).BYTES_PER_ELEMENT); + this.addAttribute(`${prefix || "typedarray"}.byteLength`, obj.byteLength); + this.addAttribute(`${prefix || "typedarray"}.byteOffset`, obj.byteOffset); return; }
224-226: Consider improving type safety in processValueThe
as anycast weakens type safety. Since we've already checked that value is an object or function, we can pass it directly.if (typeof value === "object" || typeof value === "function") { - this.doFlatten(value as any, prefix); + this.doFlatten(value, prefix); } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/hello-world/src/trigger/example.tsis excluded by!references/**
📒 Files selected for processing (11)
.changeset/chatty-snakes-hope.md(1 hunks)apps/webapp/app/env.server.ts(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(3 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(3 hunks)packages/core/src/v3/consoleInterceptor.ts(2 hunks)packages/core/src/v3/limits.ts(1 hunks)packages/core/src/v3/logger/taskLogger.ts(2 hunks)packages/core/src/v3/utils/flattenAttributes.ts(1 hunks)packages/core/src/v3/utils/ioSerialization.ts(2 hunks)packages/core/src/v3/workers/taskExecutor.ts(3 hunks)packages/core/test/flattenAttributes.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
`**/*.{ts,tsx}`: Always prefer using isomorphic code like fetch, ReadableStream,...
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
Use strict mode
No default exports, use function declarations
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
packages/cli-v3/src/entryPoints/dev-run-worker.tsapps/webapp/app/env.server.tspackages/core/src/v3/workers/taskExecutor.tspackages/core/src/v3/logger/taskLogger.tspackages/cli-v3/src/entryPoints/managed-run-worker.tspackages/core/src/v3/utils/ioSerialization.tspackages/core/src/v3/consoleInterceptor.tspackages/core/src/v3/limits.tspackages/core/test/flattenAttributes.test.tspackages/core/src/v3/utils/flattenAttributes.ts
`apps/webapp/**/*.ts`: In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
apps/webapp/**/*.ts: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/env.server.ts
`apps/webapp/**/*.{ts,tsx}`: When importing from `@trigger.dev/core` in the weba...
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/env.server.ts
`{packages/core,apps/webapp}/**/*.{ts,tsx}`: We use zod a lot in packages/core and in the webapp
{packages/core,apps/webapp}/**/*.{ts,tsx}: We use zod a lot in packages/core and in the webapp
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/app/env.server.tspackages/core/src/v3/workers/taskExecutor.tspackages/core/src/v3/logger/taskLogger.tspackages/core/src/v3/utils/ioSerialization.tspackages/core/src/v3/consoleInterceptor.tspackages/core/src/v3/limits.tspackages/core/test/flattenAttributes.test.tspackages/core/src/v3/utils/flattenAttributes.ts
`**/*.test.{ts,tsx}`: Our tests are all vitest
**/*.test.{ts,tsx}: Our tests are all vitest
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
packages/core/test/flattenAttributes.test.ts
🧠 Learnings (7)
packages/cli-v3/src/entryPoints/dev-run-worker.ts (3)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : The `trigger.config.ts` file must use `defineConfig` from `@trigger.dev/sdk/v3` to configure project settings, directories, retries, telemetry, runtime, machine settings, log level, max duration, and build options.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When logging within a task, use the `logger` API from `@trigger.dev/sdk/v3` and provide a message and a key-value object as parameters.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : Global lifecycle hooks for tasks can be defined in `trigger.config.ts` to apply to all tasks.
apps/webapp/app/env.server.ts (5)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.438Z
Learning: Applies to apps/webapp/**/*.ts : In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : The `trigger.config.ts` file must use `defineConfig` from `@trigger.dev/sdk/v3` to configure project settings, directories, retries, telemetry, runtime, machine settings, log level, max duration, and build options.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.438Z
Learning: Applies to apps/webapp/app/**/*.test.{ts,tsx} : Tests should only import classes and functions from files matching `app/**/*.ts` of the webapp, and those files should not use environment variables; everything should be passed through as options instead.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them within the `task` function in Trigger.dev task files.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When triggering a task from backend code, use `tasks.trigger`, `tasks.batchTrigger`, or `tasks.triggerAndPoll` from `@trigger.dev/sdk/v3` and set the `TRIGGER_SECRET_KEY` environment variable.
.changeset/chatty-snakes-hope.md (6)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the `task` function from `@trigger.dev/sdk/v3` and NEVER use the deprecated `client.defineJob` pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : You MUST use `@trigger.dev/sdk/v3` when writing Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When logging within a task, use the `logger` API from `@trigger.dev/sdk/v3` and provide a message and a key-value object as parameters.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `metadata` API from `@trigger.dev/sdk/v3` to attach, update, and access structured metadata within task runs.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `runs` and `tasks` APIs from `@trigger.dev/sdk/v3` to subscribe to run updates and implement realtime features.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Schema tasks must use `schemaTask` from `@trigger.dev/sdk/v3` and validate payloads using a schema (e.g., Zod).
packages/core/src/v3/logger/taskLogger.ts (3)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When logging within a task, use the `logger` API from `@trigger.dev/sdk/v3` and provide a message and a key-value object as parameters.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them within the `task` function in Trigger.dev task files.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : The `trigger.config.ts` file must use `defineConfig` from `@trigger.dev/sdk/v3` to configure project settings, directories, retries, telemetry, runtime, machine settings, log level, max duration, and build options.
packages/cli-v3/src/entryPoints/managed-run-worker.ts (3)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : The `trigger.config.ts` file must use `defineConfig` from `@trigger.dev/sdk/v3` to configure project settings, directories, retries, telemetry, runtime, machine settings, log level, max duration, and build options.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When logging within a task, use the `logger` API from `@trigger.dev/sdk/v3` and provide a message and a key-value object as parameters.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.528Z
Learning: Applies to trigger.config.ts : Global lifecycle hooks for tasks can be defined in `trigger.config.ts` to apply to all tasks.
packages/core/src/v3/utils/ioSerialization.ts (1)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.994Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : We use zod a lot in packages/core and in the webapp
packages/core/test/flattenAttributes.test.ts (2)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T13:21:33.994Z
Learning: Applies to **/*.test.{ts,tsx} : Our tests are all vitest
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-06-30T13:21:59.438Z
Learning: Applies to apps/webapp/app/**/*.test.{ts,tsx} : Tests should only import classes and functions from files matching `app/**/*.ts` of the webapp, and those files should not use environment variables; everything should be passed through as options instead.
🧬 Code Graph Analysis (4)
packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
packages/core/src/v3/limits.ts (1)
OTEL_LOG_ATTRIBUTE_COUNT_LIMIT(18-21)
packages/cli-v3/src/entryPoints/managed-run-worker.ts (1)
packages/core/src/v3/limits.ts (1)
OTEL_LOG_ATTRIBUTE_COUNT_LIMIT(18-21)
packages/core/src/v3/consoleInterceptor.ts (1)
packages/core/src/v3/utils/flattenAttributes.ts (1)
flattenAttributes(6-14)
packages/core/src/v3/utils/flattenAttributes.ts (3)
internal-packages/tracing/src/index.ts (1)
Attributes(15-15)apps/webapp/app/v3/marqs/concurrencyMonitor.server.ts (1)
key(103-170)packages/core/src/v3/index.ts (1)
NULL_SENTINEL(46-46)
🪛 Biome (1.9.4)
packages/core/src/v3/utils/ioSerialization.ts
[error] 359-359: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 361-361: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 362-362: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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). (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (26)
.changeset/chatty-snakes-hope.md (1)
1-6: LGTM! Changeset accurately describes the improvements.The changeset correctly documents the performance and memory usage improvements for logging large objects, which aligns with the broader changes in the codebase.
apps/webapp/app/env.server.ts (1)
286-287: LGTM! Consistent increase in OpenTelemetry attribute limits.The 4x increase in both span and log attribute count limits (from 256 to 1024) properly supports the improved logging of large objects. The changes are consistent and align with the PR objectives.
packages/core/src/v3/workers/taskExecutor.ts (3)
18-18: LGTM! Proper import of the attribute count limit constant.The import of
OTEL_SPAN_ATTRIBUTE_COUNT_LIMITis correctly added and will be used to enforce attribute limits in the flattening operations.
720-722: LGTM! Correctly applying attribute count limit to global init hook results.The
flattenAttributescall now properly includes theOTEL_SPAN_ATTRIBUTE_COUNT_LIMITparameter, ensuring that span attributes from global init hooks respect the configured limits.
758-760: LGTM! Correctly applying attribute count limit to task-specific init hook results.The
flattenAttributescall now properly includes theOTEL_SPAN_ATTRIBUTE_COUNT_LIMITparameter, ensuring that span attributes from task-specific init hooks respect the configured limits.packages/core/src/v3/utils/ioSerialization.ts (2)
2-14: LGTM! Proper import reorganization and additions.The imports have been appropriately reorganized to include the necessary modules for the enhanced attribute flattening functionality.
355-357: LGTM! Correctly applying attribute count limit to JSON packet attributes.The
flattenAttributescall now properly includes theOTEL_SPAN_ATTRIBUTE_COUNT_LIMITparameter, ensuring that flattened attributes respect the configured limits.packages/cli-v3/src/entryPoints/managed-run-worker.ts (3)
17-17: LGTM: Proper import of attribute count limit.The import correctly adds the
OTEL_LOG_ATTRIBUTE_COUNT_LIMITconstant from the core package.
186-188: LGTM: Consistent parameter passing to ConsoleInterceptor.The
OTEL_LOG_ATTRIBUTE_COUNT_LIMITis properly passed as the fourth parameter to the ConsoleInterceptor constructor, maintaining consistency with the constructor signature.
196-196: LGTM: Proper configuration of OtelTaskLogger.The
maxAttributeCountproperty is correctly set toOTEL_LOG_ATTRIBUTE_COUNT_LIMIT, ensuring that task logging respects the attribute count limits.packages/core/src/v3/consoleInterceptor.ts (2)
14-16: LGTM: Well-designed optional parameter addition.The optional
maxAttributeCountparameter maintains backward compatibility while enabling the new attribute count limiting functionality.
96-99: LGTM: Correct usage of attribute count limit in log flattening.The
maxAttributeCountparameter is properly passed toflattenAttributeswhen processing parsed JSON log values, ensuring that the attribute count limits are respected during console log interception.packages/cli-v3/src/entryPoints/dev-run-worker.ts (3)
18-18: LGTM: Consistent import across worker entry points.The import of
OTEL_LOG_ATTRIBUTE_COUNT_LIMITis correctly added, maintaining consistency with the managed-run-worker.ts file.
195-197: LGTM: Proper parameter passing to ConsoleInterceptor.The
OTEL_LOG_ATTRIBUTE_COUNT_LIMITis correctly passed to the ConsoleInterceptor constructor, ensuring consistent behavior across both dev and managed worker entry points.
205-205: LGTM: Consistent OtelTaskLogger configuration.The
maxAttributeCountproperty is properly set, ensuring that both dev and managed workers apply the same attribute count limits.packages/core/src/v3/logger/taskLogger.ts (2)
19-19: LGTM: Well-designed optional configuration property.The optional
maxAttributeCountproperty is properly added to theTaskLoggerConfigtype, maintaining backward compatibility.
82-87: LGTM: Improved attribute flattening with proper limit enforcement.The direct use of
flattenAttributeswith themaxAttributeCountparameter is a good improvement. The helpful comment explains the change from JSON processing to direct flattening, and the implementation correctly respects the attribute count limits.packages/core/src/v3/limits.ts (1)
16-16: LGTM: Appropriate increase in default attribute count limits.The default limits are increased from 256 to 1024 for both span and log attributes, which should improve performance when logging large objects while maintaining reasonable bounds. The 4x increase is well-balanced and consistent across both types of attributes.
Also applies to: 20-20
packages/core/test/flattenAttributes.test.ts (3)
160-183: Good addition of TypeScript error suppressionThe addition of
@ts-expect-errorcomments is appropriate for these test cases where circular references are intentionally created for testing purposes. The tests correctly verify that circular references are detected and marked with the sentinel value.
185-271: Comprehensive test coverage for maxAttributeCountExcellent test coverage for the new
maxAttributeCountparameter. The tests effectively verify:
- Basic limit enforcement
- Behavior with nested objects and arrays
- Edge cases (undefined limit, zero limit)
- Interaction with primitive values
273-477: Excellent coverage of JavaScript special typesThe test suite now comprehensively covers a wide range of JavaScript types including Error objects, functions, Set/Map collections, File objects, streams, promises, and typed arrays. Each test verifies the specific properties that should be extracted for each type, ensuring the flattening logic handles these objects appropriately.
packages/core/src/v3/utils/flattenAttributes.ts (5)
6-14: Good architectural refactoring to class-based approachThe refactoring to use the
AttributeFlattenerclass improves encapsulation and state management. The change from specific types tounknownfor theobjparameter makes the function more flexible while maintaining type safety through runtime checks.
76-83: Consider the prefix logic for Error objectsThe current implementation uses
${prefix || "error"}which might create unexpected keys when a prefix is provided. For example, with prefix "data.error", you'd get "data.error.name" instead of "error.name".Consider whether this is the intended behavior or if you want to always use "error" as the base prefix for Error properties:
- this.addAttribute(`${prefix || "error"}.name`, obj.name); - this.addAttribute(`${prefix || "error"}.message`, obj.message); + const errorPrefix = prefix ? `${prefix}.error` : "error"; + this.addAttribute(`${errorPrefix}.name`, obj.name); + this.addAttribute(`${errorPrefix}.message`, obj.message); if (obj.stack) { - this.addAttribute(`${prefix || "error"}.stack`, obj.stack); + this.addAttribute(`${errorPrefix}.stack`, obj.stack); }
92-175: Well-structured handling of special JavaScript typesThe implementation provides comprehensive support for various JavaScript types with appropriate property extraction:
- Set/Map collections are properly iterated
- File objects expose relevant metadata
- Streams indicate their type and locked state
- TypedArrays expose buffer information
The consistent use of default prefixes ensures meaningful keys in the flattened output.
27-38: Efficient attribute count limiting implementationThe implementation correctly checks the attribute count before each addition, ensuring the limit is never exceeded. The early returns throughout the code prevent unnecessary processing once the limit is reached.
176-186: Proper circular reference detectionGood use of
WeakSetfor tracking seen objects, which prevents memory leaks by allowing garbage collection. The implementation correctly checks for circular references before processing objects and adds them to the seen set appropriately.
| const { deserialize } = await loadSuperJSON(); | ||
|
|
||
| const deserialized = deserialize(data) as any; | ||
| const jsonify = safeJsonParse(JSON.stringify(deserialized, makeSafeReplacer())); | ||
|
|
||
| return imposeAttributeLimits(flattenAttributes(jsonify, undefined)); | ||
| return imposeAttributeLimits( | ||
| flattenAttributes(jsonify, undefined, OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT) | ||
| ); |
There was a problem hiding this comment.
Fix variable declarations in switch statement.
The static analysis tool correctly identifies that variable declarations in switch cases can be erroneously accessed by other cases. This is a legitimate issue that should be addressed.
Apply this diff to fix the switch statement variable scoping:
case "application/super+json":
- const { deserialize } = await loadSuperJSON();
-
- const deserialized = deserialize(data) as any;
- const jsonify = safeJsonParse(JSON.stringify(deserialized, makeSafeReplacer()));
-
- return imposeAttributeLimits(
- flattenAttributes(jsonify, undefined, OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT)
- );
+ {
+ const { deserialize } = await loadSuperJSON();
+
+ const deserialized = deserialize(data) as any;
+ const jsonify = safeJsonParse(JSON.stringify(deserialized, makeSafeReplacer()));
+
+ return imposeAttributeLimits(
+ flattenAttributes(jsonify, undefined, OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT)
+ );
+ }🧰 Tools
🪛 Biome (1.9.4)
[error] 359-359: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 361-361: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 362-362: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In packages/core/src/v3/utils/ioSerialization.ts around lines 359 to 366, the
variable declarations inside the switch statement have incorrect scoping, which
can cause variables to be accessed across cases improperly. To fix this, wrap
each case's code block in curly braces to create a new block scope, ensuring
variables declared in one case are not accessible in others.
ericallam
changed the title
No description provided.