-
Notifications
You must be signed in to change notification settings - Fork 34
feat(traceloop-sdk): standalone span processor #596
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
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 4802f7c in 52 seconds. Click for details.
- Reviewed
206
lines of code in3
files - Skipped
1
files when reviewing. - Skipped posting
8
draft 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/sample-app/package.json:23
- Draft comment:
Added new script 'run:sample_otel_sdk' and dependency '@langchain/aws'. Ensure these additions are fully tested and documented in the README for end-users. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
2. packages/traceloop-sdk/src/lib/node-server-sdk.ts:16
- Draft comment:
Exporting span-processor module; ensure its API remains stable as usage increases. - Reason this comment was not posted:
Confidence changes required:10%
<= threshold50%
None
3. packages/traceloop-sdk/src/lib/tracing/index.ts:265
- Draft comment:
Delegated span processor creation to createSpanProcessor; verify that custom attribute handling from removed onStart/onEnd hooks is correctly implemented in the new module. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that custom attribute handling is correctly implemented in the new module. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
4. packages/sample-app/package.json:24
- Draft comment:
Added 'run:sample_otel_sdk' script. Ensure that the corresponding sample file (dist/src/sample_otel_sdk.js) exists and follows expected conventions. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. packages/sample-app/package.json:44
- Draft comment:
Added dependency '@langchain/aws'. Verify its version compatibility with other langchain packages in the project. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. packages/traceloop-sdk/src/lib/node-server-sdk.ts:16
- Draft comment:
Re-exporting the new span processor module is clear. Confirm that tests cover its integration. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. packages/traceloop-sdk/src/lib/tracing/index.ts:265
- Draft comment:
Refactored span processor creation: manual onStart and onEnd logic was replaced by createSpanProcessor. Ensure the new module replicates prior attribute enrichment (e.g., setting workflow/entity attributes, span renaming, telemetry capture) as needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that the new module replicates prior attribute enrichment. This is a request for confirmation of behavior, which violates the rule against asking the PR author to ensure behavior is intended. Therefore, this comment should be removed.
8. packages/traceloop-sdk/src/lib/tracing/index.ts:288
- Draft comment:
Typo: Consider replacing 're-consider' with 'reconsider' in the comment to improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_LcePHaGvzB6GnC2C
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.
Caution
Changes requested ❌
Reviewed ab2e577 in 2 minutes and 16 seconds. Click for details.
- Reviewed
279
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
9
draft 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/sample-app/src/sample_otel_sdk.ts:23
- Draft comment:
sdk.start() returns a promise; consider awaiting its resolution to ensure proper initialization before proceeding. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment appears technically correct - start() does return a Promise. However, looking at the full context, the main application code is in an async function that's called after start(), and the SDK shutdown is properly awaited at the end. The OpenTelemetry docs don't strictly require awaiting start(), and many example implementations don't await it. The risk of race conditions seems low since the main code is scheduled to run after start() is called. I might be underestimating the importance of proper SDK initialization. There could be edge cases where traces are dropped if the SDK isn't fully initialized. While awaiting start() would be more proper, there's no strong evidence that not awaiting it causes actual problems, and many examples don't await it. The comment feels more like a style suggestion than a critical issue. The comment should be removed as it suggests a change without clear evidence that it's necessary, and many implementations don't follow this pattern.
2. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:115
- Draft comment:
Overriding span name via (span as any).name is a workaround; consider using supported SDK APIs or documenting the hack clearly. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:140
- Draft comment:
Consider logging or handling JSON parsing errors in the catch block for 'ai.prompt.messages' to aid debugging. - 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% This is a telemetry/observability system processing span attributes. Silent failure seems intentional here since this is non-critical data transformation. Adding logging could create noise in logs for what are likely benign formatting issues. The code gracefully degrades by keeping the original attribute format. The comment has a point - logging errors can help with debugging. Without logs, issues in the message format could go unnoticed. However, this is a secondary observability system - adding error logging here could create more problems than it solves by polluting application logs with non-critical formatting issues. The silent skip appears to be an intentional design choice for graceful degradation. Adding error logging would likely create unnecessary noise.
4. packages/sample-app/src/sample_langchain_bedrock.ts:21
- Draft comment:
Missing trailing newline at end of file. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply points out a missing newline, which is a minor formatting issue that does not affect the functionality of the code.
5. packages/sample-app/src/sample_otel_sdk.ts:23
- Draft comment:
Consider awaiting sdk.start() to ensure tracing is fully initialized before subsequent spans are created. - 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% Looking at OpenTelemetry docs,start()
is indeed async and returns a Promise. However, the SDK initialization is typically very fast and the main function where spans are created runs after this line. The SDK will queue any spans created before full initialization. This is more of a theoretical best practice than a real issue. The comment points out a real async operation that's not being awaited. In theory, this could cause race conditions or missed spans if the initialization takes longer than expected. While technically correct, OpenTelemetry is designed to handle this case gracefully. The impact is minimal and the code will work correctly in practice. This comment, while technically accurate, doesn't highlight a significant issue that needs fixing. It's more of an optimization suggestion without clear benefits.
6. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:12
- Draft comment:
Potential typo: 'ASSOCATION_PROPERTIES_KEY' may be intended as 'ASSOCIATION_PROPERTIES_KEY'. - Reason this comment was not posted:
Comment was on unchanged code.
7. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:116
- Draft comment:
Modifying span.name via a cast to 'any' is a workaround; consider using a supported API if available to change span names. - 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 suggests using a "supported API" but doesn't specify what that API would be. The code comments indicate the author is already aware this is a workaround and that span.name is intentionally not writable. Without evidence of what the "supported API" would be, this comment may not be actionable. The author likely already explored proper APIs before resorting to this workaround. I could be wrong in assuming the author already explored alternatives. There might be a proper API that both the author and I are unaware of. While possible, the burden of proof is on the reviewer to demonstrate a better solution exists. Without specifying the alternative API, the comment isn't actionable. Delete the comment since it points out a known limitation without providing an actionable alternative solution.
8. packages/sample-app/src/sample_langchain_bedrock.ts:21
- Draft comment:
A trailing newline is missing at the end of the file. It is a minor stylistic issue but it's best practice to end a file with a newline. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While technically correct, this is an extremely minor stylistic issue. Most modern IDEs and git configurations handle this automatically. This kind of minor stylistic issue should be handled by linting rules if the team cares about it, not PR comments. It doesn't affect functionality at all. The missing newline could cause issues with some tools or shell scripts that expect files to end with newlines. Some teams consider this an important convention. While true, this is still too minor to warrant a PR comment. If this is important to the team, it should be enforced via linting rules rather than manual review comments. Delete this comment as it's too minor of a stylistic issue to warrant a PR comment. This should be handled by linting rules if the team cares about it.
9. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:12
- Draft comment:
Typographical error: 'ASSOCATION_PROPERTIES_KEY' appears to be misspelled. Consider renaming it to 'ASSOCIATION_PROPERTIES_KEY' for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_fPYD2aTNdPWQivuA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
console.log(response); | ||
} | ||
|
||
void main().then(() => { |
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.
Consider handling errors from main()
explicitly rather than using void main().then(...)
. If main()
rejects, the error might be missed.
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.
Skipped PR review on 67db2ba because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
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.
Skipped PR review on 119a578 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
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.
Skipped PR review on 67f005b because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
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.
Skipped PR review on 2cfe3f7 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
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 99d740c in 2 minutes and 11 seconds. Click for details.
- Reviewed
237
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
12
draft 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/sample-app/src/sample_otel_sdk.ts:61
- Draft comment:
Consider handling shutdown more gracefully rather than calling process.exit(0) inside the promise chain. This can hide errors during shutdown. - Reason this comment was not posted:
Comment was on unchanged code.
2. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:57
- Draft comment:
Good use of environment fallback for the baseUrl; verify that appending '/v1/traces' consistently matches API requirements. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:133
- Draft comment:
Avoid mutating span properties (like span name) directly if possible; consider using supported APIs or documenting the workaround clearly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:142
- Draft comment:
Wrap JSON.parse in a try/catch is good; consider logging a debug message when JSON parsing fails to aid troubleshooting. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. packages/sample-app/src/sample_otel_sdk.ts:47
- Draft comment:
Verify that the 'logprobs' parameter is supported by the Chat API. Chat completion endpoints may not accept 'logprobs', so confirm it is valid. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to confirm the validity of a parameter with the API, which violates the rule against asking for confirmation or verification of intentions. It does not provide a specific code suggestion or point out a specific issue with the code itself.
6. packages/sample-app/src/sample_otel_sdk.ts:43
- Draft comment:
Consider removing redundant 'await' in 'return await withWorkflow(...)'. Simply returning the promise can simplify the async code. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. packages/sample-app/src/sample_otel_sdk.ts:62
- Draft comment:
Using process.exit in the shutdown sequence can abruptly terminate pending operations. Ensure this behavior is acceptable in your sample app. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
8. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:57
- Draft comment:
Ensure that 'options.baseUrl' does not have a trailing slash to avoid double slashes in the constructed URL. - 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% While the suggestion is technically correct - trailing slashes could cause issues - there's no evidence in the code that this is actually causing problems. The default URL "https://api.traceloop.com" doesn't have a trailing slash. Modern URL parsers and APIs generally handle double slashes gracefully. This feels like a speculative "what if" comment rather than addressing a real issue. The comment identifies a potential edge case that could cause problems in production. URL handling best practices do suggest normalizing URLs. However, this falls into the category of speculative comments - "if someone provides a URL with a trailing slash, then it could be an issue." We don't have evidence this is causing problems. Delete the comment as it's speculative and not addressing a demonstrated issue. If URL handling becomes a problem, it can be addressed when there's evidence of the need.
9. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:133
- Draft comment:
Overriding the span name by casting to 'any' is a workaround. Confirm that this mutation won’t interfere with downstream integrations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:157
- Draft comment:
Consider logging an error inside the catch block when JSON parsing fails for 'ai.prompt.messages' to assist in debugging. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
11. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:87
- Draft comment:
In onSpanStart, ensure that context values (workflow name, entity name) are valid before setting attributes. Consider adding type checks if needed. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
12. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:12
- Draft comment:
Typographical error: The constant 'ASSOCATION_PROPERTIES_KEY' appears to be misspelled. Consider renaming it to 'ASSOCIATION_PROPERTIES_KEY' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_L36m5aIJLisX9NOG
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.
Important
Looks good to me! 👍
Reviewed 575fbd2 in 1 minute and 17 seconds. Click for details.
- Reviewed
136
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
8
draft 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/sample-app/src/sample_langchain_bedrock.ts:21
- Draft comment:
Good fix: added a newline at the end of the file for consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. packages/sample-app/src/sample_otel_sdk.ts:4
- Draft comment:
Improved multi-line formatting in the import block enhances readability. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. packages/sample-app/src/sample_otel_sdk.ts:64
- Draft comment:
Promise chain formatting is now clearer. Ensure that process.exit() is acceptable in all environments. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:56
- Draft comment:
Consistent parameter formatting in createSpanProcessor improves maintainability. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. packages/sample-app/src/sample_langchain_bedrock.ts:19
- Draft comment:
Ensure the file ends with a newline for POSIX compliance. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. packages/sample-app/src/sample_otel_sdk.ts:4
- Draft comment:
The multi-line import formatting has been improved for consistency and readability. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the formatting change, which violates the rule against making purely informative comments.
7. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:56
- Draft comment:
Refactored the createSpanProcessor function signature and removed extraneous whitespace to improve code clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, describing what was done without providing any actionable feedback or suggestions. It does not align with the rules for useful comments.
8. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:12
- Draft comment:
The constant 'ASSOCATION_PROPERTIES_KEY' appears to be misspelled. It likely should be 'ASSOCIATION_PROPERTIES_KEY' to align with the correct spelling of 'association'. Please double-check and update if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_UtA8Ti0qEb6wr6xu
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Introduces a standalone span processor for Traceloop SDK with new sample applications for OpenTelemetry and Langchain Bedrock integration.
createSpanProcessor
inspan-processor.ts
for custom span handling.sample_otel_sdk.ts
demonstrating OpenTelemetry SDK integration with Traceloop span processor.sample_langchain_bedrock.ts
for Langchain Bedrock integration.package.json
to include new scripts and dependencies for@langchain/aws
.span-processor
innode-server-sdk.ts
andindex.ts
.This description was created by
for 575fbd2. You can customize this summary. It will automatically update as commits are pushed.