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.

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

[OPIK-695] sdk sentry integration #1045

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

alexkuzmik
Copy link
Collaborator

@alexkuzmik alexkuzmik commented Jan 14, 2025

Details

Added errors reporting to Sentry.
Unhandled errors inherited from opik.rest_api.core.ApiError and opik.exceptions.OpikException are reported to Sentry together with all Opik log messages with levels "error" and "warning".

Client-side configured event limits per python session - 50 warnings and 50 errors.

Testing

Unit tests are added for some sentry setup logic (logger setup + filters chain setup).
Some manual tests are performed.

@alexkuzmik alexkuzmik self-assigned this Jan 14, 2025
@alexkuzmik alexkuzmik requested review from a team as code owners January 14, 2025 15:23
@alexkuzmik alexkuzmik marked this pull request as draft January 14, 2025 15:23
@alexkuzmik alexkuzmik marked this pull request as ready for review January 14, 2025 17:17


class FilterByCount(event_filter.EventFilter):
def __init__(self, max_count: int, level: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there are only a few specific level values. Maybe we could we use Literal instead of str here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, will do!


@functools.lru_cache
def randomized_should_enable_reporting() -> bool:
return random.random() <= SESSION_REPORTING_PROBABILITY
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this func always returns True?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, in case we need to randomly sample user sessions which will have reporting enabled, we'll reduce the session reporting probability constant value.

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