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

Add Linux distro meta-data to OS context. #943

Open
supervacuus opened this issue Feb 2, 2024 Discussed in #942 · 6 comments · May be fixed by #963
Open

Add Linux distro meta-data to OS context. #943

supervacuus opened this issue Feb 2, 2024 Discussed in #942 · 6 comments · May be fixed by #963

Comments

@supervacuus
Copy link
Collaborator

Discussed in #942

Originally posted by tkatsoulas February 1, 2024
Hello guys,

The current implementation to understand the host OS https://github.com/getsentry/sentry-native/blob/master/src/sentry_os.c provides poor information for any distro. Are there any plans to update them? For instance /etc/os-releases file is there for most* of the Linux distros and can provide detailed information about a host. The current mechanisms can be used as a fallback.

Thanks in advance,
Tasos.

CC: @kahest would be interesting if other SDKs already have a tag reserved for this information.

@supervacuus supervacuus added enhancement New feature or request help wanted We won't work on this but would accept PRs good first issue Good for newcomers Platform: Linux area: core labels Feb 2, 2024
@kahest
Copy link
Member

kahest commented Feb 15, 2024

related: #467 (comment)

@kahest kahest removed the good first issue Good for newcomers label Feb 15, 2024
@supervacuus
Copy link
Collaborator Author

Hi @tkatsoulas, can I ask you: would you want to use this for searching and filtering issues (requiring us to index the field in the backend and consider cardinality), or would it suffice as an additional data item that you look up per event? Thx!

@tkatsoulas
Copy link

tkatsoulas commented Feb 15, 2024

I think both. Your current pipelines will work if we just transform the current os and os.name to something more useful rather than
Screenshot from 2024-02-15 18-29-29

If more information about a distro (like OS: Linux OS.name: Fedora OS.version:38 and the kernel-name: ... seperately) can be indexed it will be much appreciated.

This can give you a concrete view of what distros are affected by the same/similar issue.

Didn't had the time to work on the open-source contribution but I think I can have a draft PR by Monday.

@supervacuus
Copy link
Collaborator Author

supervacuus commented Feb 19, 2024

Hi @tkatsoulas, thanks. I expected that you want to be able to filter it; I just wanted to make sure.

Changing the meaning of existing os-context properties is not a good idea: it would immediately split existing events from future events, which currently would appear in the same filter results.

Some customers want to differentiate between broad OS categories (Win, macOS, Linux, etc.), and os.name is the right os-context key for this; changing its meaning would invalidate existing dashboards.

A new key (+ sub-keys?) for Linux distributions would be better, and we will talk about this internally with the ingest team about the right path.

Please wait with any time investment on your part until this is clarified since this isn't solely a client SDK issue.

@supervacuus supervacuus linked a pull request Mar 7, 2024 that will close this issue
7 tasks
@supervacuus supervacuus removed the help wanted We won't work on this but would accept PRs label Apr 30, 2024
narsaynorath pushed a commit to getsentry/sentry that referenced this issue 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 issue 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
@p0358
Copy link

p0358 commented Jun 3, 2024

One can also add Wine metadata to the context if the app is ran under that:

{
    // Add WINE version metadata if running under WINE
    HMODULE hntdll;
    typedef const char* (CDECL* wine_get_version_t)();
    typedef const char* (CDECL* wine_get_build_id_t)();
    typedef void (CDECL* wine_get_host_version_t)(const char** sysname, const char** release);
    wine_get_version_t wine_get_version;
    wine_get_build_id_t wine_get_build_id;
    wine_get_host_version_t wine_get_host_version;
    if ((hntdll = GetModuleHandleA("ntdll.dll")) && (wine_get_version = reinterpret_cast<wine_get_version_t>(GetProcAddress(hntdll, "wine_get_version"))))
    {
        sentry_value_t wine_ctx = sentry_value_new_object();

        sentry_value_set_by_key(wine_ctx, "version", sentry_value_new_string(wine_get_version())); // example: 7.11
        if (wine_get_build_id = reinterpret_cast<wine_get_build_id_t>(GetProcAddress(hntdll, "wine_get_build_id")))
            sentry_value_set_by_key(wine_ctx, "build", sentry_value_new_string(wine_get_build_id())); // example: wine-7.11
        if (wine_get_host_version = reinterpret_cast<wine_get_host_version_t>(GetProcAddress(hntdll, "wine_get_host_version")))
        {
            const char* sysname; // example: Linux
            const char* release; // example: 5.16.20-2-MANJARO
            wine_get_host_version(&sysname, &release);
            sentry_value_set_by_key(wine_ctx, "sysname", sentry_value_new_string(sysname));
            sentry_value_set_by_key(wine_ctx, "release", sentry_value_new_string(release));
        }

        sentry_set_context("wine", wine_ctx);
    }
}

@supervacuus
Copy link
Collaborator Author

One can also add Wine metadata to the context if the app is ran under that:

The effort here is definitely not in the Native SDK but in everything behind that. This issue and related PRs are not fixed/released yet because there are still issues with the lookup of the new context format in clickhouse. Let's track this separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: No status
Status: Backlog
Development

Successfully merging a pull request may close this issue.

4 participants