Skip to content
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.

Sign up for GitHub

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

Jump to bottom

OPIK-72 Add batch remove traces endpoint #256

Merged
merged 2 commits into from
Sep 17, 2024
Merged

OPIK-72 Add batch remove traces endpoint #256

merged 2 commits into from
Sep 17, 2024

Conversation

andrescrz
Copy link
Collaborator

@andrescrz andrescrz commented Sep 16, 2024 •
edited
Loading

Details

Added endpoint to delete traces by a set of UUIDs. It has the same logic as the existing method for a single UUID, that means:

  • Feedback scores related to the traces are deleted in cascade.
  • Spans related to the traces are deleted in cascade.
  • Feedback scores of the spans related to the traces are deleted in cascade.
  • Then, the traces are deleted.

Issues

OPIK-72

Testing

  • Added integration tests covering all the logic explained above for the delete traces endpoint.
  • Added integration tests covering all the logic explained above for the deleted trace endpoint.
  • All cases manually tested.
  • Found and fixed multiple issues in the existing tests.
  • Refactors and improvements in the related resouce integration test classes.

Documentation

N/A

@andrescrz andrescrz self-assigned this Sep 16, 2024
@andrescrz andrescrz marked this pull request as ready for review September 16, 2024 17:21
@andrescrz andrescrz requested a review from a team as a code owner September 16, 2024 17:21
thiagohora
thiagohora approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@thiagohora thiagohora left a comment

Choose a reason for hiding this comment

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

Great job on the tests.

I just remembered that we are missing the deletion of experiment items related to a trace. Otherwise, the feedback score counts and avg will be impacted

@andrescrz andrescrz merged commit e715f5c into main Sep 17, 2024
2 checks passed
@andrescrz andrescrz deleted the andrescrz/OPIK-72-add-batch-remove-traces-endpoint branch September 17, 2024 07:49
@andrescrz
Copy link
Collaborator Author

Great job on the tests.

I just remembered that we are missing the deletion of experiment items related to a trace. Otherwise, the feedback score counts and avg will be impacted

Discussed offline and decided to not to cascade deletions to experiment items.

dsblank pushed a commit that referenced this pull request Oct 4, 2024
@andrescrz
OPIK-72 Add batch remove traces endpoint (#256)
4aee011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagohora thiagohora thiagohora approved these changes

Assignees

@andrescrz andrescrz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andrescrz @thiagohora