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

feat: include error message in HaystackError telemetry events #3543

Merged
merged 6 commits into from
Nov 10, 2022
Merged

feat: include error message in HaystackError telemetry events #3543

merged 6 commits into from
Nov 10, 2022

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Nov 8, 2022

Related Issues

Proposed Changes:

Added a message field to all HaystackError(s) raised. If set message property will be visible in telemetry

How did you test it?

Manual verification, there is no other way.

Notes for the reviewer

Someone familiar with telemetry needs to review. Otherwise, the context is rather big and requires time to pick up.

Checklist

@vblagoje vblagoje requested a review from a team as a code owner November 8, 2022 13:59
@vblagoje vblagoje requested review from mayankjobanputra and julian-risch and removed request for a team and mayankjobanputra November 8, 2022 13:59
@julian-risch
Copy link
Member

julian-risch commented Nov 9, 2022

@vblagoje I committed the small changes that we discussed. In addition, one could add the send_message_in_event parameter also to FilterError with default None. That would allow to raise a FilterError and explicitly turn on send_message_in_event to send the message in the telemetry event if that is needed somewhere in Haystack. However, I don't see a need for that at the moment and therefore I didn't include it. Please also review the changes and if you approve them too that this is ready to be merged. 👍

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 @vblagoje please also review the changes, thank you. Feel free to merge when you are done with your review.

@vblagoje
Copy link
Member Author

vblagoje commented Nov 9, 2022

Yes, LGTM as well. If CI is green, let's integrate it!

@julian-risch julian-risch merged commit 005025b into deepset-ai:main Nov 10, 2022
@julian-risch julian-risch changed the title Use telemetry to identify most common errors feat: include error message in HaystackError telemetry events Nov 10, 2022
@vblagoje vblagoje deleted the telemetry_error branch March 31, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants