-
-
Notifications
You must be signed in to change notification settings - Fork 463
feat: minimal tombstone integration #4933
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: main
Are you sure you want to change the base?
feat: minimal tombstone integration #4933
Conversation
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java
Outdated
Show resolved
Hide resolved
|
@markushi, I just realized I cannot omit having a separate "tombstone" marker, even if I report all events, without repeating them. I mean, this was clear to me, but, in addition to it being a must for this PR already, unlike the ANR marker, I must also align it with I wonder if it would make the most sense to do it similarly to The biggest issue with that approach is that the The PR still makes sense for a first review from you (since, if the general direction makes sense and you have todos not in my list, I can also add a test for the integration itself). Still, I would appreciate a short sync on how to align these execution paths (maybe I don't need to align tombstones with I can convert the PR back to a draft if you prefer. |
markushi
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.
Left a few comments - great work so far!
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Outdated
Show resolved
Hide resolved
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Outdated
Show resolved
Hide resolved
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Outdated
Show resolved
Hide resolved
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Show resolved
Hide resolved
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/proto/io/sentry/android/core/internal/tombstone/tombstone.proto
Show resolved
Hide resolved
| otel-semconv = { module = "io.opentelemetry.semconv:opentelemetry-semconv", version.ref = "otelSemanticConventions" } | ||
| otel-semconv-incubating = { module = "io.opentelemetry.semconv:opentelemetry-semconv-incubating", version.ref = "otelSemanticConventionsAlpha" } | ||
| p6spy = { module = "p6spy:p6spy", version = "3.9.1" } | ||
| protobuf-javalite = { module = "com.google.protobuf:protobuf-javalite", version.ref = "protobuf"} |
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.
@romtsn Not sure if you followed the conversations, but as you can see here, protobuf requires a runtime dependency. It will have an impact of around 10kb. IMHO fine for now, we should still check how stable this library is to avoid and consumer version mismatch issues.
Co-authored-by: Markus Hintersteiner <m.hintersteiner@gmail.com>
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
… S, since that is the earliest version where `REASON_CRASH_NATIVE` provides tombstones via the `traceInputStream`)
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Outdated
Show resolved
Hide resolved
…rker This currently does not work: While we now can optionally enable reporting of "historical" tombstones, by making the `TombstoneHint` `Backfillable` it will automatically be enriched by the `ANRv2EventProcessor` which is currently the only `BackfillingEventProcessor`. The `ANRv2EventProcessor` is partially written in way that is potentially generic for events with `Backfillable` hints, but other parts are enriching as if those are always were ANRs, which up to now was true, but with Tombstones that assumption now breaks. Next Steps: * There is considerable duplication between the ANRv2Integration and TombstoneIntegration
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Show resolved
Hide resolved
…ns via HistoryDispatcher * also update api dumps and formatting for sentry changes
...ry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java
Outdated
Show resolved
Hide resolved
…tProcessor this handles all events with Backfillable hint, but adds an interface HintEnricher, to allow hint-specific enrichment (like for ANRs) before and after the generic backfilling happened.
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java
Show resolved
Hide resolved
| @NotNull SentryEvent event, @NotNull Backfillable hint, @NotNull Object rawHint); | ||
| } | ||
|
|
||
| private final class AnrHintEnricher implements HintEnricher { |
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.
That sounds like a pretty good solution to me, nice!
sentry-android-core/src/main/java/io/sentry/android/core/TombstoneEventProcessor.java
Outdated
Show resolved
Hide resolved
| private @Nullable ApplicationExitInfo removeLatest( | ||
| final @NotNull List<ApplicationExitInfo> exitInfos) { | ||
| for (ApplicationExitInfo applicationExitInfo : exitInfos) { | ||
| if (applicationExitInfo.getReason() == policy.getTargetReason()) { | ||
| exitInfos.remove(applicationExitInfo); | ||
| return applicationExitInfo; | ||
| } | ||
| } | ||
| return null; | ||
| } |
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.
Not if we exit the loop right after the removal. This is verbatim from the initial code. If we want to remove a potential foot gun RE: future changes, I can make the iterator explicit so that the removal updates both the iterator and the data structure.
… iterator instead of for-each
📜 Description
As discussed last week with @markushi, the process will be to make minimal atomic changes in each PR and merge directly to
mainrather than accumulating on an uber feature branch. This allows for easier review/feedback/corrections, and we can already test a subset of the entire feature "in the field".The first minimal change is a basic tombstone integration:
sentry-android-core) or get two reports for the same crashApplicationExitInfoentries withREASON_CRASH_NATIVEor report them too, including the option for the latter; I left this out for minimal interface exposure in the first step, but either variant is easy to add to this PR or later)protobuf-javalite(the entire features adds ca. 75KiB to the Android sample release APK)protobufplugin and theprotoccompiler to automate protocol updatesOpen Issues:
While theprotobufruntime is relatively small, there is still the possibility of conflicting with client-sideprotobufversions (major versions often introduce quite severe breakage, but I haven't tested this yet, only reviewed change logs).finding clarity in how to proceed with older tombstones (ignore or handling similarly toANRv2)decision if this minimal setup already makes sense to release as an internal APIManifestMetadataReaderto configure conveniently? (or not since the corresponding options are only internal?)💡 Motivation and Context
First sensible release step for #3295
Part of https://linear.app/getsentry/project/tombstone-support-android-0024cb6e3ffd/
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
protobufruntime library as a dependency tosentry-android-core(or shade/relocate, or implement our own decoder, given that this is a stable format which only requires a subset ofprotobuf).EventProcessorthat merges crash events fromsentry-nativewith tombstones.#skip-changelog (for now)