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(native): Set sdk.name for events created from minidumps #4313

Merged

Conversation

tobias-wilfert
Copy link
Member

@tobias-wilfert tobias-wilfert commented Nov 29, 2024

Sets the sdk.name for events created out of minidumps to minidump, mimicking the logic that we currently have for unreal.

Not sure we can add more detail than just minidump? Presumably something like this could work but I am not sure if we want to do that:

  let more_detailed_sdk_name = if minidump.get_stream::<MinidumpCrashpadInfo>().is_ok() {
      "minidump.crashpad"
  } else if minidump.get_stream::<minidump::MinidumpBreakpadInfo>().is_ok() {
      "minidump.breakpad"
  } else {
      "minidump.unknown"
  };

Fix: #4236

@tobias-wilfert tobias-wilfert requested a review from a team as a code owner November 29, 2024 10:44
};
use relay_protocol::{Annotated, Value};

type Minidump<'a> = minidump::Minidump<'a, &'a [u8]>;

/// Client SDK name used for the event payload to identify events created out of minidumps.
const CLIENT_SDK_NAME: &str = "minidump";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would give this a name to indicate that it applies specifically to minidumps.

@loewenheim
Copy link
Contributor

Adding the more specific names sounds like a good idea to me.

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Would be good if you modify existing tests to also assert the client sdk name, we should have at least some tests that can be easily extended to assert the SDK name.

@tobias-wilfert tobias-wilfert merged commit c724228 into master Nov 29, 2024
23 checks passed
@tobias-wilfert tobias-wilfert deleted the tobias-wilfert/feat/add-sdk-name-to-minidump-events branch November 29, 2024 13:58
};

// Add sdk information for analytics.
event.client_sdk.get_or_insert_with(|| ClientSdkInfo {
Copy link
Member

Choose a reason for hiding this comment

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

will this override the SDK set in the client? Or this only affects minidumps from the minidump endpoint?

Meaning if it's sentry-native attaching a minidump from crashpad, will it no longer send sentry-native?

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 sdk.name to events created out of minidumps
4 participants