-
-
Notifications
You must be signed in to change notification settings - Fork 464
feat(envelope-item): Support span type #4935
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
|
| CheckIn("check_in"), | ||
| Feedback("feedback"), | ||
| Log("log"), | ||
| Span("span"), | ||
| Unknown("__unknown__"); // DataCategory.Unknown | ||
|
|
||
| private final String itemType; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
valid concern but I am not sure if we need to handle this now since we cannot capture spans directly from the java sdk anyway
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.
hm, I think it's still valid even for the hybrid SDKs because client reports are counted from the unsent envelop items? But anyway, it can be done in a follow up PR later
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'll double check this
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 380.65 ms | 451.27 ms | 70.62 ms |
| a416a65 | 295.53 ms | 373.74 ms | 78.21 ms |
| 889ecea | 367.58 ms | 437.52 ms | 69.94 ms |
| a5ab36f | 320.47 ms | 389.77 ms | 69.30 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| ee747ae | 400.46 ms | 423.61 ms | 23.15 ms |
| d217708 | 375.27 ms | 415.68 ms | 40.41 ms |
| fcec2f2 | 357.47 ms | 447.32 ms | 89.85 ms |
| 23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
| d5a29b6 | 298.62 ms | 391.78 ms | 93.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 604a261 | 1.58 MiB | 2.10 MiB | 533.42 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 889ecea | 1.58 MiB | 2.11 MiB | 539.75 KiB |
| a5ab36f | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| 23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
| d5a29b6 | 1.58 MiB | 2.12 MiB | 549.37 KiB |
Previous results on branch: feat/support-span-sentry-item-type
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 598bacc | 298.68 ms | 352.42 ms | 53.74 ms |
| c357e8a | 408.90 ms | 554.76 ms | 145.86 ms |
| a110e56 | 297.40 ms | 373.86 ms | 76.45 ms |
| d9d541c | 329.45 ms | 357.49 ms | 28.04 ms |
| e9c7b57 | 321.04 ms | 392.09 ms | 71.05 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 598bacc | 1.58 MiB | 2.13 MiB | 557.33 KiB |
| c357e8a | 1.58 MiB | 2.13 MiB | 557.38 KiB |
| a110e56 | 1.58 MiB | 2.13 MiB | 557.49 KiB |
| d9d541c | 1.58 MiB | 2.13 MiB | 557.33 KiB |
| e9c7b57 | 1.58 MiB | 2.13 MiB | 557.34 KiB |
Moved 'Support span envelope item type' feature to internal section.
romtsn
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.
LGTM, maybe create a followup issue to address the client reports/rate limiter concern?
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
|
@romtsn updated the impl to handle span datacategory for client report / rate limiter as well double checked relay and "span" datacategory is corret here |
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
romtsn
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.
re-checked -- looks good!
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
📜 Description
Span First initiative - this PR is needed for Dart/Flutter right now but will be needed in general in the future when Java also implements span first
💡 Motivation and Context
See span protocol https://develop.sentry.dev/sdk/telemetry/spans/span-protocol/
Needed for processing envelope items with type
span.Currently needed in Flutter's span first implementation as we rely on Java's
InternalSentrySdk.captureEnvelope. Without this support the envelope item type is set tounknownwhen you usecaptureEnvelopeand the span won't be ingested💚 How did you test it?
Unit test and manually tested together with the Sentry Flutter SDK
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps