search

getsentry / sentry-java

A Sentry SDK for Java, Android and other JVM languages.
https://docs.sentry.io/
MIT License
1.15k stars 434 forks source link

Send client reports on Sentry.close #2139

Open buckett opened 2 years ago

buckett commented 2 years ago

Problem Statement

I think that as outlined in https://develop.sentry.dev/sdk/client-reports/ reports are attached to existing envelopes that are sent to sentry.

Our problem is that in some of our uses of sentry the JVM is running a "job" and after completing the "job" the JVM exits. We are seeing a problem sending a transaction to sentry (because it's too large), but after that failure the JVM exits and so the client report failure never gets sent to sentry (so the transaction gets dropped and it's not easy to see we're getting dropped transactions because the client report is never sent).

Solution Brainstorm

If there are outstanding client reports add a shutdown hook to the JVM that sends them to sentry.

philipphofmann commented 2 years ago

By design, client report is a best-effort feature, see https://develop.sentry.dev/sdk/client-reports/#scope-and-intent

That being said we are not looking to perfectly measure every nuance and edge case of events being discarded in SDKs. It is more important to have a best effort and be able to gain insights to our SDKs and their host applications.

We don't expect the numbers to be 100% accurate. Anyways, I think it wouldn't harm if send outstanding client reports when calling Sentry.close(). What do you think @adinauer, @mattjohnsonpint?

Then you could hook it up yourself. Would that work for you, @buckett?

buckett commented 2 years ago

Completely get that it's a best efforts approach (and FWIW think that's the way to go), but I wanted to highlight that if it's not a long running process failing errors may well not get reported to Sentry.

Yeah, flushing any outstanding clients reports on Sentry.close() seems like a good approach to me.

adinauer commented 2 years ago

Sending client reports on Sentry.close() sounds like a good improvement to me.

@buckett as a workaround until we get around to improving this, you could try to generate a dummy event, so the client report is sent alongside that before shutdown happens.

mattjohnsonpint commented 2 years ago

This sounds reasonable to me as well. Thanks.

mattjohnsonpint commented 2 years ago

FYI, I added this to the .NET SDK and found it better to implement when flushing the queue. Just afterwards, to account for discarded events while flushing (rate limiting, etc.). Not sure if you'll encounter the same in Java. See https://github.com/getsentry/sentry-dotnet/pull/1757