Skip to content

Conversation

@dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented May 28, 2025

Important

Add trace tagging functionality to both asynchronous and synchronous clients and update version to 0.6.4.

  • Behavior:
    • Adds AsyncTags class in tags.py for asynchronous trace tagging.
    • Adds Tags class in tags.py for synchronous trace tagging.
    • Integrates AsyncTags and Tags into AsyncLaminarClient and LaminarClient respectively.
    • Adds get_trace_id() method to Laminar class to retrieve current trace ID.
  • Version:
    • Updates version to 0.6.4 in pyproject.toml and version.py.

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

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: https://app.greptile.com/review/github.

9 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

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 everything up to 4b9fe0c in 1 minute and 35 seconds. Click for details.
  • Reviewed 357 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 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. src/lmnr/sdk/client/synchronous/resources/tags.py:56
  • Draft comment:
    Similar to the async version, trace_id provided as a string isn’t validated. Consider adding validation to ensure the string is a valid UUID.
  • 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% This is a new file being added. The comment points out a potential issue where invalid string UUIDs could be passed through. However, the API endpoint would likely reject invalid UUIDs anyway, and the docstring already warns about ValueError for invalid UUIDs. The validation may be happening server-side intentionally. I may be underestimating the importance of client-side validation. Invalid UUIDs could cause unnecessary API calls that fail. While client-side validation could be nice, without knowing the full system design and server-side handling, we can't be certain this is actually an issue that needs fixing. The comment raises a valid point but we lack strong evidence that this is definitely a problem requiring fixing rather than an intentional design choice.
2. src/lmnr/version.py:22
  • Draft comment:
    Consider specifying a timeout for the HTTP request in get_latest_pypi_version to avoid potential hangs if PyPI is slow to respond.
  • 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.
3. src/lmnr/sdk/laminar.py:713
  • Draft comment:
    Typographical issue: The docstring line contains an unnecessary backslash after 'None if'. Consider removing the backslash to enhance clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% 1. Backslashes for line continuation in docstrings are a valid Python syntax. 2. They help maintain consistent line lengths. 3. While not strictly necessary here since docstrings can naturally span multiple lines, they provide consistent formatting with the rest of the file. 4. Looking at the full file, backslashes are used consistently throughout other docstrings. The backslash doesn't hurt readability and matches the style used throughout the file. Removing it would make this docstring inconsistent with others. The comment suggests a style change that would make this docstring inconsistent with the rest of the codebase. Consistency is more important here. The backslash should be kept to maintain consistent formatting with other docstrings in the file.

Workflow ID: wflow_fRKYcUeZ7sp7vQfZ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

dinmukhamedm and others added 2 commits May 28, 2025 16:00
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
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 1e83801 in 1 minute and 22 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 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.

Workflow ID: wflow_QsRtS5a2tSSU86KY

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

elif isinstance(trace_id, int):
trace_id = str(uuid.UUID(int=trace_id))
elif isinstance(trace_id, str):
uuid.UUID(trace_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider converting the validated UUID string into its canonical form. Instead of just calling uuid.UUID(trace_id), assign the result (e.g. trace_id = str(uuid.UUID(trace_id))) for consistency with how other types are handled.

Suggested change
uuid.UUID(trace_id)
trace_id = str(uuid.UUID(trace_id))

@dinmukhamedm dinmukhamedm merged commit a2a91a1 into main May 28, 2025
6 checks passed
@dinmukhamedm dinmukhamedm deleted the tagging-traces branch May 28, 2025 16:09
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.

2 participants