feat(webapp): add ecs otel resource attributes #2454
Conversation
|
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 2
🧹 Nitpick comments (7)
internal-packages/run-engine/src/engine/locking.ts (1)
195-196: Avoid letting caller attributes clobber canonical fieldsRight now, user-supplied attributes can override name/resources/timeout. Prefer canonical fields to win, or namespace caller attrs (e.g., custom.*).
Apply this diff to change precedence:
- attributes: { name, resources, timeout: this.duration, ...attributes }, + attributes: { ...(attributes ?? {}), name, resources, timeout: this.duration },internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1)
576-586: Consider OTel naming convention for attributesDotted, namespaced keys (e.g., trigger.run.id) are easier to query than snake_case custom keys.
Example:
- run_id: runId, + "trigger.run.id": runId,apps/webapp/app/v3/telemetry/asyncResourceAttributes.server.ts (4)
167-176: Handle image digests and multi-colon namesImage can be repo@sha256:..., or have multiple colons. Safer parsing:
- if (containerData.Image) { - const [name, tag] = containerData.Image.split(":"); + if (containerData.Image) { + // prefer tag after last colon if not using digest form + const atIndex = containerData.Image.indexOf("@"); + const imageRef = atIndex >= 0 ? containerData.Image.slice(0, atIndex) : containerData.Image; + const lastColon = imageRef.lastIndexOf(":"); + const name = lastColon >= 0 ? imageRef.slice(0, lastColon) : imageRef; + const tag = lastColon >= 0 ? imageRef.slice(lastColon + 1) : undefined;
237-239: Reduce log noise: downgrade to debugFetching ECS metadata every boot can spam logs.
- logger.info("🔦 Fetched ECS metadata", { attributes }); + logger.debug("🔦 Fetched ECS metadata", { attributes });
54-56: Optional: coalesce concurrent fetchesPrevent duplicate network calls on first access by memoizing the in-flight Promise.
Add alongside ecsMetadataCache:
let ecsMetadataPromise: Promise<ResourceAttributes> | null = null;Then in fetchECSMetadata():
if (ecsMetadataCache) return ecsMetadataCache; if (ecsMetadataPromise) return ecsMetadataPromise; ecsMetadataPromise = (async () => { // existing body that computes `attributes` ecsMetadataCache = attributes; ecsMetadataPromise = null; return attributes; })(); return ecsMetadataPromise;Also applies to: 207-241
232-235: Schema additions needed in env.server.tsTo support the above, add optional fields to EnvironmentSchema:
Add to apps/webapp/app/env.server.ts:
AWS_REGION: z.string().optional(), AWS_DEFAULT_REGION: z.string().optional(), ECS_CONTAINER_METADATA_URI_V4: z.string().optional(),Want me to open a follow-up PR for this?
Also applies to: 255-263
apps/webapp/app/v3/tracer.server.ts (1)
203-204: Reuse a single Resource instance to avoid divergence and extra async workCreate it once inside setupTelemetry and pass to all providers.
Apply these diffs:
- const provider = new NodeTracerProvider({ - forceFlushTimeoutMillis: 15_000, - resource: getResource(), + const resource = getResource(); + const provider = new NodeTracerProvider({ + forceFlushTimeoutMillis: 15_000, + resource,- const loggerProvider = new LoggerProvider({ - resource: getResource(), + const loggerProvider = new LoggerProvider({ + resource,- const meterProvider = new MeterProvider({ - resource: getResource(), + const meterProvider = new MeterProvider({ + resource,Also applies to: 253-254, 308-309
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/webapp/app/v3/telemetry/asyncResourceAttributes.server.ts(1 hunks)apps/webapp/app/v3/tracer.server.ts(6 hunks)internal-packages/run-engine/src/engine/locking.ts(2 hunks)internal-packages/run-engine/src/engine/systems/dequeueSystem.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{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
No default exports, use function declarations
Files:
internal-packages/run-engine/src/engine/locking.tsapps/webapp/app/v3/telemetry/asyncResourceAttributes.server.tsapps/webapp/app/v3/tracer.server.tsinternal-packages/run-engine/src/engine/systems/dequeueSystem.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/v3/telemetry/asyncResourceAttributes.server.tsapps/webapp/app/v3/tracer.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
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.
Files:
apps/webapp/app/v3/telemetry/asyncResourceAttributes.server.tsapps/webapp/app/v3/tracer.server.ts
🧬 Code graph analysis (4)
internal-packages/run-engine/src/engine/locking.ts (2)
internal-packages/run-engine/src/run-queue/index.ts (2)
T(997-1026)name(272-274)internal-packages/tracing/src/index.ts (1)
Attributes(15-15)
apps/webapp/app/v3/telemetry/asyncResourceAttributes.server.ts (1)
apps/webapp/app/v3/tracer.server.ts (1)
logger(104-109)
apps/webapp/app/v3/tracer.server.ts (2)
apps/webapp/app/env.server.ts (1)
env(1099-1099)apps/webapp/app/v3/telemetry/asyncResourceAttributes.server.ts (1)
getAsyncResourceAttributes(255-263)
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1)
internal-packages/run-engine/src/run-queue/index.ts (6)
message(1395-1443)message(1692-1747)message(1749-1797)message(1799-1827)message(1848-1861)workerQueue(1557-1690)
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
internal-packages/run-engine/src/engine/locking.ts (1)
147-152: Nice: lock() accepts span attributes without breaking callersOptional attributes on the 4th param keeps the API backward-compatible and enables richer spans.
Also applies to: 195-196
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (2)
146-154: Good: useful span context for dequeue operationRun, org, env, queue, consumer, and blocking flags on the span help root-cause latency.
576-586: blocking_pop default matches run-queue
No changes required.apps/webapp/app/v3/tracer.server.ts (1)
174-183: LGTM: centralized Resource with host name and async attributesGood consolidation and use of async resource enrichment.
Also, the runlock span now takes additional attributes