Skip to content

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

Merged
merged 8 commits into from
Apr 22, 2025

Conversation

galkleinman
Copy link
Contributor

@galkleinman galkleinman commented Apr 21, 2025

Important

Introduces a standalone span processor for Traceloop SDK with new sample applications for OpenTelemetry and Langchain Bedrock integration.

  • Span Processor:
    • Introduces createSpanProcessor in span-processor.ts for custom span handling.
    • Handles span start and end events, adapting attributes for Vercel AI compatibility.
  • Sample Applications:
    • Adds sample_otel_sdk.ts demonstrating OpenTelemetry SDK integration with Traceloop span processor.
    • Adds sample_langchain_bedrock.ts for Langchain Bedrock integration.
  • Package Updates:
    • Updates package.json to include new scripts and dependencies for @langchain/aws.
    • Exports span-processor in node-server-sdk.ts and index.ts.

This description was created by Ellipsis for 575fbd2. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

console.log(response);
}

void main().then(() => {
Copy link
Contributor

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.

@galkleinman galkleinman changed the title feat(traceloop-sdk): standalone span processor feat(traceloop-sdk): standalone span processor (WIP) Apr 21, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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.

@galkleinman galkleinman changed the title feat(traceloop-sdk): standalone span processor (WIP) feat(traceloop-sdk): standalone span processor Apr 22, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@galkleinman galkleinman merged commit 05a6326 into main Apr 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for initializing span processors and exporters without initializing the NodeSDK
2 participants