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: add linux distributions to os context #963

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

supervacuus
Copy link
Collaborator

@supervacuus supervacuus commented Mar 7, 2024

Fixes #943.

This adds Linux distribution meta-data to the os context as follows:

Screenshot 2024-03-07 at 14 55 00

Only name and version should be indexed.

Further steps required before we merge/release this:

Copy link

github-actions bot commented Mar 7, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 1699009

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

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

Project coverage is 82.55%. Comparing base (235f837) to head (1699009).
Report is 65 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #963      +/-   ##
==========================================
- Coverage   86.53%   82.55%   -3.98%     
==========================================
  Files          40       53      +13     
  Lines        3736     7860    +4124     
  Branches        0     1232    +1232     
==========================================
+ Hits         3233     6489    +3256     
- Misses        503     1260     +757     
- Partials        0      111     +111     

@supervacuus
Copy link
Collaborator Author

The failing Mingw test is unrelated to the change (a change in the GHA runner image requires us to install zlib for MinGW).

@supervacuus supervacuus force-pushed the feat/add_linux_distros_to_os_context branch from 700a65a to 44866bb Compare March 20, 2024 18:05
@supervacuus
Copy link
Collaborator Author

@markushi, @Swatinem, can one of you give this a thorough look?

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Left a few minor nits, looking good!

src/sentry_os.c Outdated Show resolved Hide resolved
tests/unit/test_os_release.c Outdated Show resolved Hide resolved
src/sentry_os.c Show resolved Hide resolved
src/sentry_os.c Outdated Show resolved Hide resolved
src/sentry_os.c Show resolved Hide resolved
@supervacuus
Copy link
Collaborator Author

again, failing test here is due to: #968

narsaynorath pushed a commit to getsentry/sentry that referenced this pull request May 17, 2024
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
cmanallen pushed a commit to getsentry/sentry that referenced this pull request May 21, 2024
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
@supervacuus supervacuus mentioned this pull request May 22, 2024
5 tasks
@supervacuus supervacuus marked this pull request as draft June 17, 2024 09:33
@supervacuus
Copy link
Collaborator Author

PR is on hold until the context format is clarified. Nested context formats with three layers are unacceptable for the backend to search, so we either change the format in the SDK or map it locally in the sentry backend.

Since the format was at the core of how to add this feature from the start, it is best to change the format in the SDK (and with it, in the develop-docs, relay, and sentry backend).

This is currently deprioritized.

@JoshuaMoelans JoshuaMoelans marked this pull request as ready for review November 26, 2024 09:12
JoshuaMoelans added a commit to getsentry/relay that referenced this pull request Nov 28, 2024
…4292)

(continuation of #3443 )

<!-- Describe your PR here. -->
To make these fields searchable, we had to change from a nested to a
flattened approach. This is provided in sentry-native ([relevant
PR](getsentry/sentry-native#963)).

The update is also tracked on the docs side:
getsentry/sentry-docs#11936
JoshuaMoelans added a commit to getsentry/sentry that referenced this pull request Nov 28, 2024
<!-- Describe your PR here. -->
update to #69865
To search the distribution fields, we cannot have them in a hierarchy
`os.distribution.name`, we instead need them flattened to
`os.distribution_name`

This is part of reworking the following native-SDK PR:
getsentry/sentry-native#963

The update is also tracked on the docs side:
getsentry/sentry-docs#11936
And on Relay: getsentry/relay#4292
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.

Add Linux distro meta-data to OS context.
4 participants