feat: Add os context to events automatically#467
Conversation
Swatinem
force-pushed
the
feat/os-context
branch
from
30667b0 to
188843f
Compare
|
By the way, maybe is it worth for Linux to parse |
|
@Mixaill I agree that the linux kernel version on its own has little meaning, however this is consistent with the information we can extract from breakpad/crashpad minidumps. |
|
I've seen crashpad include a custom section with lsb-release and friends, so at least there would be a possibility to do this. |
|
@flub uname gives the following: The |
|
|
||
| void *ffibuf = NULL; | ||
|
|
||
| DWORD size = GetFileVersionInfoSizeW(L"kernel32.dll", NULL); |
There was a problem hiding this comment.
Why kernel32.dll version is used instead of GetVersionExW()?
Libraries versions does not match the Windows build version.
(yeah, there is popular advice in the internet to use kernel32.dll but it is not correct)
There was a problem hiding this comment.
Again, I copied whatever crashpad is doing, to be consistent with those tools. I was a bit surprised myself that the version does not reflect the 20H2 release number.
I will discuss this internally.
There was a problem hiding this comment.
Also, you can use values from HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion (that's what winver.exe actually use)
- Version:
<CurrentMajorVersionNumber>.<CurrentMinorVersionNumber> - Full Build:
<CurrentMajorVersionNumber>.<CurrentMinorVersionNumber>.<CurrentBuildNumber>.<UBR> - Edition:
<EditionID> - Update name:
<DisplayVersion>
There was a problem hiding this comment.
Also, you can combine GetVersionExW and registry to detect if application is running under compatibility mode:
- In case of compatibility mode,
GetVersionExWreturns target version, not the real one) - In case of missing
supportedOSfield in application manifest,GetVersionExWreturns 6.2 instead of 10.0
|
@Swatinem any further work is planned in a short time or can I improve behavior of Windows version detection? |
Well, I call this solved from my end. The thing is that I built this to have the same behavior as crashpad. They also have a comment related to this here: https://github.com/getsentry/crashpad/blob/cb520bd41fdea18a4d842efabeac61a2a5beaa61/snapshot There should be a compelling reason to deviate from that. I haven’t looked at the specific code in breakpad yet, but I expect it to do something similar. |
|
Breakpad is using windbg APIs for this: https://docs.microsoft.com/en-us/windows/win32/api/minidumpapiset/nf-minidumpapiset-minidumpwritedump I will see what that gives me in terms on windows version… |
|
So that minidump gives me |
|
But, in any case, if I provide PR improvements in this area, will they be accepted or reviewed at least? |
|
@jan-auer was a bit hesitant, maybe you can elaborate your concerns a bit more? I have no objections if its equally well supported and a clear improvement. |
4 participants
This captures OS context for the major OS (windows, macos, linux).
Q:
version: 11.1, should I append a trailing.0if the patch version is missing?5.4.61-android11-0-00791-gbad091cc4bf3-ab6833933, should I split that off into abuildidentifier rather?