-
-
Notifications
You must be signed in to change notification settings - Fork 897
feat(otel): support for custom resource attributes via config#telemetry.resource and OTEL_RESOURCE_ATTRIBUTES env var #2704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 81bfa44 The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
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 pull request implements support for custom OpenTelemetry resource properties in trigger.dev. It introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
…ry.resource and OTEL_RESOURCE_ATTRIBUTES env var
d8fc22f to
81bfa44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/webapp/app/v3/otlpExporter.server.ts (1)
307-323: LGTM! Consistent attribute extraction for spans.The extraction logic for span resource attributes mirrors the log extraction, ensuring consistent behavior. While there's some code duplication with the log extraction logic, it's acceptable given the different contexts.
Consider extracting the common resource attribute filtering logic into a shared helper function to reduce duplication:
function extractUserDefinedResourceAttributes( resourceAttributes: KeyValue[], spanAttributeValueLengthLimit: number ) { return truncateAttributes( convertKeyValueItemsToMap(resourceAttributes ?? [], [], undefined, [ SemanticInternalAttributes.USAGE, SemanticInternalAttributes.SPAN, SemanticInternalAttributes.METADATA, SemanticInternalAttributes.STYLE, SemanticInternalAttributes.METRIC_EVENTS, SemanticInternalAttributes.TRIGGER, "process", "sdk", "service", "ctx", "cli", "cloud", ]), spanAttributeValueLengthLimit ); }This would eliminate duplication between
convertLogsToCreateableEventsandconvertSpansToCreateableEvents.apps/webapp/app/presenters/v3/SpanPresenter.server.ts (1)
488-515: Span properties/resourceProperties serialization is safe and UI-friendlyConditionally JSON‑stringifying only when
span.properties/span.resourcePropertiesare non‑empty objects avoids noisy empty blocks in the UI and protects against null/undefined. As long as these fields remain object-shaped on the backend, this is solid.packages/core/src/v3/otel/tracingSDK.ts (1)
505-574: Consider makingparseOtelResourceAttributesmore forgiving of bad inputThe parser is nicely constrained (length + printable ASCII, percent‑decoding, invalid
key=valuepairs skipped), but invalid keys/values or a single malformed percent‑encoding currently throw and will abortTracingSDKconstruction whenCUSTOM_OTEL_RESOURCE_ATTRIBUTESis set.You might want to instead log/diag‑warn and skip only the offending pair so a single bad attribute doesn’t prevent the worker from starting, e.g. by catching decode errors and falling back to
continuerather thanthrow.apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (2)
171-191: Attributes + resourceAttributes merge on write side looks consistent
createEventToTaskEventV1Inputnow funnels bothevent.propertiesandevent.resourcePropertiesthroughcreateEventToTaskEventV1InputAttributes, which:
- strips private keys,
- unflattens,
- and, for resources, wraps them under a
$resourcekey.The empty‑object fallback when both inputs are missing keeps the ClickHouse payload clean. Call sites that only pass
propertiesremain compatible via the optional second parameter.
398-421: Helper composition for public vs. resource attributes is clearThe split between
createEventToTaskEventV1InputAttributesandcreateAttributesToInputAttributesis readable and avoids duplication: public attributes become top‑level fields; resource attributes become a$resourcesub‑object when present.Given
createAttributesToInputAttributesalready handlesundefinedand empty cases, the merged object is predictable and safe to JSON‑encode downstream.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreferences/telemetry/package.jsonis excluded by!references/**references/telemetry/src/trigger/tasks.tsis excluded by!references/**references/telemetry/trigger.config.tsis excluded by!references/**references/telemetry/tsconfig.jsonis excluded by!references/**
📒 Files selected for processing (13)
.changeset/lucky-cameras-shave.md(1 hunks)apps/webapp/app/presenters/v3/SpanPresenter.server.ts(1 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx(1 hunks)apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts(2 hunks)apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts(5 hunks)apps/webapp/app/v3/eventRepository/eventRepository.types.ts(2 hunks)apps/webapp/app/v3/otlpExporter.server.ts(4 hunks)packages/cli-v3/src/dev/devSupervisor.ts(0 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(1 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(1 hunks)packages/cli-v3/src/utilities/dotEnv.ts(2 hunks)packages/core/src/v3/config.ts(2 hunks)packages/core/src/v3/otel/tracingSDK.ts(5 hunks)
💤 Files with no reviewable changes (1)
- packages/cli-v3/src/dev/devSupervisor.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T09:49:07.011Z
Learnt from: julienvanbeveren
Repo: triggerdotdev/trigger.dev PR: 2417
File: apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.import.ts:56-61
Timestamp: 2025-08-19T09:49:07.011Z
Learning: In the Trigger.dev codebase, environment variables should default to `isSecret: false` when not explicitly marked as secrets in the syncEnvVars functionality. This is the intended behavior for both regular variables and parent variables.
Applied to files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
🧬 Code graph analysis (5)
apps/webapp/app/v3/eventRepository/eventRepository.types.ts (1)
internal-packages/tracing/src/index.ts (1)
Attributes(15-15)
apps/webapp/app/v3/otlpExporter.server.ts (2)
packages/core/src/v3/taskContext/index.ts (1)
resourceAttributes(50-69)packages/core/src/v3/semanticInternalAttributes.ts (1)
SemanticInternalAttributes(1-68)
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (4)
apps/webapp/app/v3/eventRepository/eventRepository.server.ts (2)
event(118-149)event(1089-1108)packages/core/src/v3/utils/flattenAttributes.ts (1)
attributes(23-25)internal-packages/tracing/src/index.ts (1)
Attributes(15-15)packages/core/src/v3/taskContext/index.ts (1)
resourceAttributes(50-69)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (2)
packages/core/src/v3/schemas/api.ts (1)
EnvironmentVariable(928-931)apps/webapp/app/v3/environmentVariables/repository.ts (1)
EnvironmentVariable(77-80)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (1)
apps/webapp/app/components/code/CodeBlock.tsx (1)
CodeBlock(187-412)
⏰ 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). (17)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (16)
packages/cli-v3/src/utilities/dotEnv.ts (2)
5-11: LGTM! Formatting improvement.The multi-line array format improves readability with no functional changes.
32-35: LGTM! Intentional renaming to control resource attribute processing.The transformation from
OTEL_RESOURCE_ATTRIBUTEStoCUSTOM_OTEL_RESOURCE_ATTRIBUTESprevents OpenTelemetry SDKs from automatically parsing these attributes before Trigger.dev can process them. This gives the system control over when and how custom resource attributes are applied.apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (2)
828-837: LGTM! Consistent variable renaming at the environment level.The transformation from
OTEL_RESOURCE_ATTRIBUTEStoCUSTOM_OTEL_RESOURCE_ATTRIBUTESis applied consistently with the CLI-level transformation. The placement is correct—after fetching variables but before deduplication.
860-867: LGTM! Clean and focused helper function.The
renameVariablesfunction correctly maps variable keys according to the provided mapping. The implementation is non-mutating and preserves all other variable properties.apps/webapp/app/v3/eventRepository/eventRepository.types.ts (2)
56-56: LGTM! Appropriate type addition for resource properties.The
resourceProperties?: Attributesfield correctly uses the OpenTelemetry Attributes type and is appropriately optional for backwards compatibility. The placement next to thepropertiesfield is logical.
213-213: LGTM! Appropriate type for UI rendering context.The
resourcePropertiesfield uses a permissive union type suitable for display purposes. The type aligns with the adjacentpropertiesfield and the comment clearly documents its usage in the UI.packages/core/src/v3/config.ts (2)
15-15: LGTM! Correct OpenTelemetry import.The import of the
Resourcetype from@opentelemetry/resourcesis appropriate for adding resource configuration support.
112-117: LGTM! Well-documented configuration addition.The
resourcefield is appropriately typed and documented. The optional nature maintains backwards compatibility. The documentation link follows the established pattern for this config file.apps/webapp/app/v3/otlpExporter.server.ts (3)
207-223: LGTM! Appropriate filtering of user-defined resource attributes.The extraction logic correctly filters out internal Trigger.dev attributes (using semantic internal attribute prefixes) and standard OpenTelemetry system attributes (process, sdk, service, etc.). The truncation prevents excessive data while preserving user-defined attributes.
270-270: LGTM! Consistent resource properties propagation.The
resourcePropertiesfield correctly receives the filtered user-defined resource attributes, matching the updatedCreateEventInputtype.
376-376: LGTM! Consistent span resource properties assignment.The assignment matches the pattern used for log events and aligns with the updated type definitions.
packages/cli-v3/src/entryPoints/managed-run-worker.ts (1)
191-191: LGTM! Proper resource configuration propagation.The resource configuration is correctly passed from the trigger config to the TracingSDK initialization. The optional chaining safely handles cases where telemetry or resource might be undefined.
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (1)
1074-1084: No verification issues found.The SpanPresenter already serializes both
propertiesandresourcePropertiesto JSON strings before passing them to the route component. The serialization logic checks if the field is an object with keys, then converts it usingJSON.stringify(), otherwise returnsundefined. This means the CodeBlock component receives a properly formatted string (orundefined), and the conditional rendering in your code is correct.packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
205-213: Resource propagation into TracingSDK looks correctForwarding
config.telemetry?.resourceinto theTracingSDKconfig cleanly hooks user-defined resources into the worker’s OTEL setup; no issues spotted with ordering or optionality here.packages/core/src/v3/otel/tracingSDK.ts (1)
65-115: Resource merging strategy is coherent and extensibleAdding
resource?: ResourcetoTracingSDKConfigand merging, in order, detected resources, built‑ins, env‑driven attributes, taskContext.resourceAttributes, and finallyconfig.resourcegives a clear precedence model where user/config overrides win without losing defaults. This wiring looks consistent with how the rest of the SDK composes resources.apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
1115-1120: SpanDetail now correctly separates span vs. resource propertiesIn
#mergeRecordsIntoSpanDetail:
resourcePropertiesis initialized toundefined, preserving backward compatibility.- On the first record with
attributes_textand emptyspan.properties, you:
- parse attributes JSON,
- extract
$resourceintoresourceAttributes,- delete
$resourcefrom the remaining attributes,- assign the rest to
span.properties,- assign
resourceAttributestospan.resourceProperties.This mirrors the write‑side
$resourceconvention and ensures the presenter/UI can show span properties and resource properties separately. The “only if properties empty” guard matches the previous behavior of using the first attributes payload per span; just confirm that’s still the intended precedence if later records carry richer attributes.Also applies to: 1207-1219
It's now possible to set custom resource attributes on exported spans and logs using two different methods
Config
You can set a set of custom resource attributes via the new
resourceproperty in yourtrigger.config.tsfile:Env vars
We now support the official
OTEL_RESOURCE_ATTRIBUTESenv var (docs):Resource attributes will now show in the Span & Log inspector under "Resource properties":
Fixes #2648