-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 os.distribution searchable #69865
Make os.distribution searchable #69865
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #69865 +/- ##
==========================================
- Coverage 77.90% 77.90% -0.01%
==========================================
Files 6522 6522
Lines 290539 290555 +16
Branches 50265 50271 +6
==========================================
Hits 226344 226344
- Misses 57952 57965 +13
- Partials 6243 6246 +3
|
Okay, the failing test was due to a restriction to two path elements in the attribute name. I am not sure if this is a global restriction. If so, I'd need to rename the attributes to I quickly fixed this here: 6a2bad0, which must also be reverted if the path length should be 2. |
elif path[0] == "os": | ||
if path[1] in ("distribution"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone were to search for os.distribution
then we'd get an index error here. I think we can extend the pattern above by removing the elif len(path) != 2
check and structuring it as:
# remove the len(path) != 2 check
# everything that indexes on 2 segments of the path
if len(path) == 2:
return [] # No other attribute paths of 2 have been mapped
# The new branch for indexing os.distribution.name/version
return [] # Handles all other cases
Everything else looks good to me though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added delimiting path-length branches and a test that would have failed in the previous version with an IndexError
.
@narsaynorath, do you think the remaining failing test is related to my changes? This comes from a list comparison of project IDs; I am unsure how my changes could have affected this. |
@narsaynorath, it seems like the failing test is gone now without any change in this PR. Is there anything else I can do to merge this PR? |
@supervacuus sorry for not getting back to you! I was looking into the failed test but got pulled away. I have to set a label to trigger the backend tests but I can approve after those pass. Just to confirm, the intention is to make this field searchable in places like Discover and Dashboards? Anywhere else specifically? We'll have to make a frontend PR to surface this field |
@kahest suggested that a front-end was optional as a first step since we only use it for search. Honestly, I know very little about how Sentry works in that regard, and whether adding a front-end is required or not goes beyond my level of insight. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. I'll follow up with @kahest on Monday about what else we expect to get done for this r.e. the UI
Users of the Native SDK also want to search for the Linux distributions their events came from: getsentry/sentry-native#943 The corresponding PRs to * develop docs: getsentry/develop#1227 * relay: getsentry/relay#3443 * Native SDK: getsentry/sentry-native#963
Users of the Native SDK also want to search for the Linux distributions their events came from: getsentry/sentry-native#943
The corresponding PRs to
@markushi: please have a quick look if that makes any sense 😅
cc: @kahest