-
-
Notifications
You must be signed in to change notification settings - Fork 344
(feat-V7) Log Tracing #4827
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
base: v7
Are you sure you want to change the base?
(feat-V7) Log Tracing #4827
Conversation
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf | 420.06 ms | 435.31 ms | 15.25 ms |
9da5c4e | 478.08 ms | 467.46 ms | -10.63 ms |
fb6493a | 446.78 ms | 441.65 ms | -5.13 ms |
8a54dff | 482.57 ms | 467.16 ms | -15.41 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf | 17.75 MiB | 19.54 MiB | 1.79 MiB |
9da5c4e | 17.75 MiB | 20.16 MiB | 2.41 MiB |
fb6493a | 17.75 MiB | 19.58 MiB | 1.83 MiB |
8a54dff | 17.75 MiB | 19.54 MiB | 1.80 MiB |
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9da5c4e+dirty | 1231.84 ms | 1235.49 ms | 3.65 ms |
8a54dff+dirty | 1226.17 ms | 1227.79 ms | 1.63 ms |
9fbcfaf+dirty | 1216.66 ms | 1212.06 ms | -4.60 ms |
fb6493a+dirty | 1222.42 ms | 1235.50 ms | 13.08 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9da5c4e+dirty | 2.63 MiB | 3.76 MiB | 1.13 MiB |
8a54dff+dirty | 2.63 MiB | 3.79 MiB | 1.16 MiB |
9fbcfaf+dirty | 2.63 MiB | 3.77 MiB | 1.14 MiB |
fb6493a+dirty | 2.63 MiB | 3.78 MiB | 1.15 MiB |
Previous results on branch: lz/v7-test-logs
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee79082+dirty | 1217.93 ms | 1221.08 ms | 3.15 ms |
b592bbd+dirty | 1219.21 ms | 1232.27 ms | 13.06 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee79082+dirty | 2.63 MiB | 3.76 MiB | 1.13 MiB |
b592bbd+dirty | 2.63 MiB | 3.76 MiB | 1.13 MiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9da5c4e+dirty | 1215.41 ms | 1226.38 ms | 10.97 ms |
8a54dff+dirty | 1244.29 ms | 1254.71 ms | 10.43 ms |
9fbcfaf+dirty | 1215.22 ms | 1222.26 ms | 7.04 ms |
fb6493a+dirty | 1229.61 ms | 1221.21 ms | -8.40 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9da5c4e+dirty | 3.19 MiB | 4.33 MiB | 1.14 MiB |
8a54dff+dirty | 3.19 MiB | 4.35 MiB | 1.17 MiB |
9fbcfaf+dirty | 3.19 MiB | 4.34 MiB | 1.15 MiB |
fb6493a+dirty | 3.19 MiB | 4.35 MiB | 1.16 MiB |
Previous results on branch: lz/v7-test-logs
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee79082+dirty | 1223.58 ms | 1225.13 ms | 1.54 ms |
b592bbd+dirty | 1218.20 ms | 1229.35 ms | 11.14 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ee79082+dirty | 3.19 MiB | 4.33 MiB | 1.14 MiB |
b592bbd+dirty | 3.19 MiB | 4.33 MiB | 1.14 MiB |
build errors will be fixed on #4860 |
…into lz/v7-test-logs
…-react-native into lz/v7-test-logs
@@ -182,7 +182,7 @@ export const NATIVE: SentryNativeWrapper = { | |||
typeof itemHeader.content_type === 'string' ? itemHeader.content_type : 'application/octet-stream'; | |||
bytesPayload = itemPayload; | |||
} else { | |||
bytesContentType = 'application/json'; | |||
bytesContentType = 'application/vnd.sentry.items.log+json'; |
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 use the new content type for all requests or only the ones that contain logs? @krystofwoldrich @kahest
This can create a break changes on self -hosted, and I am not sure which version has support for logging.
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.
on Sentry JavaScript for example, they use application/vnd.sentry.items.log+json
by default on all requests
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf+dirty | 417.92 ms | 431.81 ms | 13.90 ms |
fb6493a+dirty | 352.12 ms | 372.92 ms | 20.80 ms |
8a54dff+dirty | 393.65 ms | 392.32 ms | -1.33 ms |
9da5c4e+dirty | 399.70 ms | 407.34 ms | 7.64 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf+dirty | 7.15 MiB | 8.29 MiB | 1.14 MiB |
fb6493a+dirty | 7.15 MiB | 8.33 MiB | 1.18 MiB |
8a54dff+dirty | 7.15 MiB | 8.30 MiB | 1.15 MiB |
9da5c4e+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
CHANGELOG.md
Outdated
@@ -61,6 +84,10 @@ Version 7 of the SDK is compatible with Sentry self-hosted versions 24.4.2 or hi | |||
- `@sentry/utils` package, the exports were moved to `@sentry/core` | |||
- Standalone `Client` interface & deprecate `BaseClient` | |||
|
|||
### Self Hosted | |||
|
|||
- It is recommended to use Sentry Self Hosted version `24.8.0` this and future versions. |
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 validated that capture of events/exceptions are working on 24.8.0, but we should at least recommend the first version suggested here getsentry/self-hosted#3560
CHANGELOG.md
Outdated
@@ -61,6 +84,10 @@ Version 7 of the SDK is compatible with Sentry self-hosted versions 24.4.2 or hi | |||
- `@sentry/utils` package, the exports were moved to `@sentry/core` | |||
- Standalone `Client` interface & deprecate `BaseClient` | |||
|
|||
### Self Hosted | |||
|
|||
- It is recommended to use Sentry Self Hosted version `25.2.0` or new for React Native V7 or newer |
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.
this change affects all events/transactions due to the content type of the request being changed.
25.2.0 is a safe value for it getsentry/self-hosted#3560, even if we don't officially support logs, it'll not drop other types of content
@krystofwoldrich @antonis it's ready for review :D |
To enable it add the following code to your Sentry Options: | ||
|
||
```typescript | ||
_experiments: { | ||
enableLogs: true, | ||
}, | ||
``` | ||
|
||
You can also filter the logs being collected by adding beforeSendLogs into `_experiments` | ||
|
||
```typescript | ||
_experiments: { | ||
enableLogs: true, | ||
beforeSendLog: (log) => { | ||
return log; | ||
}, | ||
}, | ||
``` |
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.
nit: I think we should align one tab to the 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.
LGTM and worked as expected in my tests 🎸
📢 Type of change
Blocked by:
📜 Description
Add the required changes on the Client in order to capture logs. All platforms will be supported with the initial PR (web, iOS and Android)
💡 Motivation and Context
Fixes #4856
💚 How did you test it?
Locally, with native SDKs disabled for testing the JavaScript integration, and with native SDKs enabled to test the Native integration
Pii
Pii seems to be filtered serverside so we don't need to do anything on the SDK.

📝 Checklist
sendDefaultPII
is enabled🔮 Next steps