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

Make error.main_thread searchable on Discover and Issues page #46183

Merged
merged 27 commits into from
Apr 11, 2023

Conversation

marandaneto
Copy link
Contributor

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 22, 2023
@marandaneto marandaneto requested a review from a team March 22, 2023 12:25
@marandaneto
Copy link
Contributor Author

Do I need to add error.main_thread into the event_search.py file boolean_keys array?

@marandaneto
Copy link
Contributor Author

Do I need to add error.main_thread into filter.py -> key_conversion_map?

@marandaneto
Copy link
Contributor Author

I'm not adding a error.main_thread tag so I believe I don't need to change the backend.py -> get_tag_value_paginator_for_projects, right?

@marandaneto
Copy link
Contributor Author

My PR is basically looking at error.handled usage, I have 0 knowledge of the Sentry codebase, help is appreciated, thanks.

@marandaneto marandaneto requested review from untitaker and evanh and removed request for a team March 27, 2023 08:19
@marandaneto
Copy link
Contributor Author

@evanh or @untitaker would any of you be able to help me out with all those open questions? thanks.

@marandaneto marandaneto requested a review from a team March 27, 2023 12:14
Copy link
Member

@wmak wmak left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the event_attribute code, but for discover all you should need to do is update src/sentry/snuba/events.py if there's no post-processing required

src/sentry/snuba/events.py Show resolved Hide resolved
@marandaneto marandaneto removed the request for review from evanh March 28, 2023 08:39
@marandaneto
Copy link
Contributor Author

marandaneto commented Mar 28, 2023

Do I need to change src/sentry/search/events/filter.py and src/sentry/search/events/datasets/discover.py? there are similar methods for isHandled, notHandled and isHandled, I just need error.main_thread:True or error.main_thread:False to work, does this need pre-post processing?

@wmak
Copy link
Member

wmak commented Mar 28, 2023

Do I need to add error.main_thread into the event_search.py file boolean_keys array?

Yes

Do I need to add error.main_thread into filter.py -> key_conversion_map?

No, filter.py is deprecated by src/sentry/search/events/datasets/discover.py, but you only need to add it there if you require special handling, eg. error.handled is actually the snuba function notHandled

Do I need to change src/sentry/search/events/filter.py and src/sentry/search/events/datasets/discover.py? there are similar methods for isHandled, notHandled and isHandled, I just need error.main_thread:True or error.main_thread:False to work, does this need pre-post processing?

No, from your other answer where this is just a boolean field, all you should need to do is update snuba.py -- see the previous answer for when you need to update those two files

@marandaneto
Copy link
Contributor Author

Do I need to add error.main_thread into the event_search.py file boolean_keys array?

Yes

Do I need to add error.main_thread into filter.py -> key_conversion_map?

No, filter.py is deprecated by src/sentry/search/events/datasets/discover.py, but you only need to add it there if you require special handling, eg. error.handled is actually the snuba function notHandled

Do I need to change src/sentry/search/events/filter.py and src/sentry/search/events/datasets/discover.py? there are similar methods for isHandled, notHandled and isHandled, I just need error.main_thread:True or error.main_thread:False to work, does this need pre-post processing?

No, from your other answer where this is just a boolean field, all you should need to do is update snuba.py -- see the previous answer for when you need to update those two files

Alright, thanks, this has to be searchable on the Alert and Issues page as well, is there anyone that knows how this work? my tests are still broken, so I'm unsure what is wrong or what I have to do.

@marandaneto
Copy link
Contributor Author

@barkbarkimashark @wmak tests are green and this is ready for approval/merging.
Thank you so much for your help.

Copy link
Member

@wmak wmak left a comment

Choose a reason for hiding this comment

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

Changes to discover code look good to me, a reminder you'll need to update the frontend too to see error.main_thread in autocompletes etc.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants