-
-
Notifications
You must be signed in to change notification settings - Fork 451
Add device and OS attributes to logs #4493
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
Conversation
|
Performance metrics 🚀
|
@@ -45,6 +45,17 @@ default SentryReplayEvent process(@NotNull SentryReplayEvent event, @NotNull Hin | |||
return event; | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to going through EventProcessor
would be something very similar to it, like a LogProcessor
so I opted for simply extending EventProcessor
instead.
We could also offer hooks but I didn't want to hack that in, so I went this route and we can transform it into a better hook API in the future.
* @return the event itself, a mutated SentryLogEvent or null | ||
*/ | ||
@Nullable | ||
default SentryLogEvent process(@NotNull SentryLogEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided not to add Hint
here since we decided to not have it for logs.beforeSend
either. LMK if you think it's needed.
@@ -199,6 +210,37 @@ private void mergeOS(final @NotNull SentryBaseEvent event) { | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I'm thinking querying DeviceInfoUtil is probably too much work for this, shall we maybe just directly use the const values for brand, model and family from android.os.Build
? Same for the OS name and version, those are also const values. Just trying to minimize impact since logs can happen frequent and originate from the main thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, introduced a LazyEvaluator
so we don't run a regex for the family on each log emitted.
* @return the event itself, a mutated SentryLogEvent or null | ||
*/ | ||
@Nullable | ||
default SentryLogEvent process(@NotNull SentryLogEvent event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add an empty override to MainEventProcessor
? I recall there used to be problems in dotnet/unity due to absence of proper desugaring in some versions, so the default interface methods have caused crashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of things to address, but LGTM otherwise!
@romtsn What's the best way for HybridSDKs to use this feature when capturing logs on the hybrid layer? |
📜 Description
Add device and OS attributes to logs:
os.name
os.device
device.brand
device.model
device.family
💡 Motivation and Context
Closes #4460
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps