sentry icon indicating copy to clipboard operation
sentry copied to clipboard

Make os.distribution searchable

Open supervacuus opened this issue 1 year ago • 2 comments

Users of the Native SDK also want to search for the Linux distributions their events came from: https://github.com/getsentry/sentry-native/issues/943

The corresponding PRs to

  • develop docs: https://github.com/getsentry/develop/pull/1227
  • relay: https://github.com/getsentry/relay/pull/3443
  • Native SDK: https://github.com/getsentry/sentry-native/pull/963

@markushi: please have a quick look if that makes any sense 😅 cc: @kahest

supervacuus avatar Apr 29 '24 14:04 supervacuus

Codecov Report

Attention: Patch coverage is 44.44444% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 77.90%. Comparing base (3ecef85) to head (9ab3ade). Report is 2 commits behind head on master.

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     
Files Coverage Δ
src/sentry/snuba/events.py 100.00% <100.00%> (ø)
src/sentry/rules/conditions/event_attribute.py 70.23% <37.50%> (-1.41%) :arrow_down:

... and 10 files with indirect coverage changes

codecov[bot] avatar Apr 29 '24 15:04 codecov[bot]

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 os.distribution_name and os.distribution_version respectively.

I quickly fixed this here: https://github.com/getsentry/sentry/pull/69865/commits/6a2bad065ce330faf1b440e405ec0e6d81531af6, which must also be reverted if the path length should be 2.

supervacuus avatar Apr 29 '24 17:04 supervacuus

@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.

supervacuus avatar May 07 '24 08:05 supervacuus

@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 avatar May 17 '24 10:05 supervacuus

@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

narsaynorath avatar May 17 '24 12:05 narsaynorath

@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.

supervacuus avatar May 17 '24 12:05 supervacuus