-
Notifications
You must be signed in to change notification settings - Fork 46
feat: Add Google Cloud destination support #814
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
This commit adds support for the Google Cloud destination of OpenLLMetry JS. - Adds the `@google-cloud/opentelemetry-cloud-trace-exporter` dependency. - Modifies the `InitializeOptions` interface to include a `gcpProjectId` property. - Updates the tracing initialization logic to use the Google Cloud exporter when a `gcpProjectId` is provided.
WalkthroughAdds the Google Cloud Trace exporter dependency, extends InitializeOptions with an optional Changes
Sequence DiagramsequenceDiagram
participant User
participant SDK as Traceloop SDK Init
participant Selector as Exporter Selector
participant GCP as GCP Trace Exporter
participant OTLP as OTLP Trace Exporter
User->>SDK: initialize(options)
SDK->>Selector: evaluate options.gcpProjectId
alt gcpProjectId provided
Selector->>GCP: instantiate with projectId
GCP-->>Selector: exporter ready
else gcpProjectId absent
Selector->>OTLP: instantiate with baseUrl/headers
OTLP-->>Selector: exporter ready
end
Selector-->>SDK: configured exporter
SDK-->>User: initialization complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
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.
Important
Looks good to me! 👍
Reviewed everything up to b9e6bae in 1 minute and 12 seconds. Click for details.
- Reviewed
56lines of code in3files - Skipped
1files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/package.json:78
- Draft comment:
Verify if this new dependency should be optional since it’s only used when gcpProjectId is provided. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment asks to "verify" something, which violates our rules about not asking for verification. We don't have enough context about how the dependency is used to know if it should be optional. The comment is speculative without strong evidence. Making dependencies optional is an implementation detail that the author likely already considered. Perhaps this is actually an important architectural decision about keeping the package size small when GCP features aren't needed. The comment could be raising a valid concern. While package size optimization is valid, we don't have enough context to know if this is correct. The comment is phrased as a verification request rather than a clear action item. The comment should be deleted as it asks for verification rather than stating a clear issue, and we lack context to know if the suggestion is correct.
2. packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.ts:146
- Draft comment:
Document the expected format or requirements for gcpProjectId to aid future users. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. packages/traceloop-sdk/src/lib/tracing/index.ts:289
- Draft comment:
When both a custom exporter and gcpProjectId are provided, the custom exporter takes precedence. Confirm that this behavior is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_C4LceQiIqUN7kk5h
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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: 1
🧹 Nitpick comments (2)
packages/traceloop-sdk/src/lib/tracing/index.ts (1)
290-291: Consider validatinggcpProjectIdis non-empty.An empty string for
gcpProjectIdwould pass the truthy check but likely cause the GCP exporter to fail. Consider adding validation to ensure it's a non-empty string before creating the exporter.Example validation:
const traceExporter = options.exporter ?? - (options.gcpProjectId + (options.gcpProjectId && options.gcpProjectId.trim() !== "" ? new GcpTraceExporter({ projectId: options.gcpProjectId }) : new OTLPTraceExporter({Or add explicit validation earlier with a clearer error message.
packages/traceloop-sdk/package.json (1)
78-78: Version ^1.0.0 is valid and secure; consider optional dependency for bundle size optimization.Version
^1.0.0exists with no known security vulnerabilities. The GCP trace exporter is conditionally used only whengcpProjectIdis provided, but currently remains a regular dependency that's always installed. To reduce bundle size for users who don't need GCP integration, consider making this an optional or peer dependency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/traceloop-sdk/package.json(1 hunks)packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.ts(1 hunks)packages/traceloop-sdk/src/lib/tracing/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/{instrumentation-*,traceloop-sdk}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Import AI/LLM semantic attribute constants from @traceloop/ai-semantic-conventions rather than hardcoding strings
Files:
packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.tspackages/traceloop-sdk/src/lib/tracing/index.ts
packages/traceloop-sdk/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/traceloop-sdk/**/*.{ts,tsx}: Use the provided decorators (@workflow, @task, @agent) for workflow/task/agent spans instead of re-implementing them
For manual LLM operations, use trace.withLLMSpan from @traceloop/node-server-sdk
Files:
packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.tspackages/traceloop-sdk/src/lib/tracing/index.ts
packages/*/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Use workspace:* for intra-repo package dependencies in package.json
Files:
packages/traceloop-sdk/package.json
packages/traceloop-sdk/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
When adding a new instrumentation package, add it to the main SDK dependencies
Files:
packages/traceloop-sdk/package.json
🧠 Learnings (1)
📚 Learning: 2025-08-24T22:08:07.023Z
Learnt from: CR
PR: traceloop/openllmetry-js#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T22:08:07.023Z
Learning: Applies to packages/traceloop-sdk/package.json : When adding a new instrumentation package, add it to the main SDK dependencies
Applied to files:
packages/traceloop-sdk/package.json
🔇 Additional comments (2)
packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.ts (1)
145-149: LGTM!The
gcpProjectIdproperty is well-documented, appropriately optional, and follows the existing interface conventions.packages/traceloop-sdk/src/lib/tracing/index.ts (1)
5-5: LGTM!The import is clean and the alias
GcpTraceExporterprovides good clarity.
This commit adds support for the Google Cloud destination of OpenLLMetry JS. - Adds the `@google-cloud/opentelemetry-cloud-trace-exporter` dependency. - Modifies the `InitializeOptions` interface to include a `gcpProjectId` property. - Updates the tracing initialization logic to use the Google Cloud exporter when a `gcpProjectId` is provided. - Fixes a bug where the OTLP exporter was not using the computed `baseUrl`.
|
@nirga I am sorry for bothering you, but can you review the pull request? |
nirga
left a comment
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.
Thanks! Sorry for the delay in reviewing - was OOO and the team missed this
|
@nirga Thank you for merging this. I appreciate if you could release a new version for the changes. |
|
@yu-iskw releasing now |
Solves #616
This commit adds support for the Google Cloud destination of OpenLLMetry JS.
@google-cloud/opentelemetry-cloud-trace-exporterdependency.InitializeOptionsinterface to include agcpProjectIdproperty.gcpProjectIdis provided.Summary by CodeRabbit