-
-
Notifications
You must be signed in to change notification settings - Fork 461
Extend Logs API to allow passing in attributes
#4402
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 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 93da41e | 429.70 ms | 484.57 ms | 54.87 ms |
| 878e381 | 402.20 ms | 417.65 ms | 15.45 ms |
| 4c45928 | 454.84 ms | 508.46 ms | 53.62 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 93da41e | 1.58 MiB | 2.08 MiB | 510.52 KiB |
| 878e381 | 1.58 MiB | 2.08 MiB | 510.51 KiB |
| 4c45928 | 1.58 MiB | 2.08 MiB | 510.86 KiB |
|
|
||
| @SuppressWarnings("AnnotateFormatMethod") | ||
| private void captureLog( | ||
| final @Nullable Map<String, Object> attributes, |
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.
I don't like it too much that it's the first param but I guess it must be like this because of the varargs right?
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.
I would like it more for this to be the last param but I don't think that's possible. Either way it's fine, I would expect most users to not go through this API
3a26d0a to
5ca032d
Compare
|
Sorry pushed on this branch by mistake, should be restored, please double check |
lcian
left a comment
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.
Did some manual tests and everything works as intended except for serialization of arbitrary objects as attributes, opened a PR on top of this to fix that #4409
| final @Nullable Object... args) { | ||
| final @NotNull HashMap<String, SentryLogEventAttributeValue> attributes = new HashMap<>(); | ||
|
|
||
| if (incomingAttributes != null) { |
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.
Should we create a copy of the attributes (here or in the capture method) to avoid having a map that could be changed while we iterate on it?
* Improve logger attributes API * Review feedback
📜 Description
Extend Logs API to allow passing in
attributes💡 Motivation and Context
Filtering Logs in Sentry via custom attributes.
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps