Skip to content
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

ref(on_crash): follow-up to make on_crash releasable #734

Merged
merged 13 commits into from
Jul 27, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ jobs:

- name: Expose llvm PATH for Mac
if: ${{ runner.os == 'macOS' }}
run: echo $(brew --prefix llvm@13)/bin >> $GITHUB_PATH
run: echo $(brew --prefix llvm@14)/bin >> $GITHUB_PATH
supervacuus marked this conversation as resolved.
Show resolved Hide resolved

- name: Installing Android SDK Dependencies
if: ${{ env['ANDROID_API'] }}
Expand Down
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,25 @@

## Unreleased
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.5.0 or 0.4.19? Moving the sampling to the end of the pre-processing (in #729), thereby executing before_send() unconditionally, could be considered a breaking change. I am fine either way.


**Features**

- Provide `on_crash()` callback to allow clients to act on detected crashes
Swatinem marked this conversation as resolved.
Show resolved Hide resolved
([#724](https://github.com/getsentry/sentry-native/pull/724))

**Fixes**

- Make Windows ModuleFinder more resilient to missing Debug Info
([#732](https://github.com/getsentry/sentry-native/pull/732))
- Aligned pre-send event processing in `sentry_capture_event()` with the
[cross-SDK session filter order](https://develop.sentry.dev/sdk/sessions/#filter-order)
([#729](https://github.com/getsentry/sentry-native/pull/729))

**Thank you**:

Features, fixes and improvements in this release have been contributed by:

- [@espkk](https://github.com/espkk)

## 0.4.18

**Features**:
Expand Down
38 changes: 38 additions & 0 deletions examples/example.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,31 @@
# define sleep_s(SECONDS) sleep(SECONDS)
#endif

static sentry_value_t
before_send_callback(sentry_value_t event, void *hint, void *closure)
{
// make our mark on the event
sentry_value_set_by_key(
event, "adapted_by", sentry_value_new_string("before_send"));

// tell the backend to proceed with the event
return event;
}

static int
discarding_on_crash_callback(const sentry_ucontext_t *uctx, void *closure)
{
// tell the backend to discard the event
return 0;
}

static int
retaining_on_crash_callback(const sentry_ucontext_t *uctx, void *closure)
{
// tell the backend to retain the event
return 1;
}

static void
print_envelope(sentry_envelope_t *envelope, void *unused_state)
{
Expand Down Expand Up @@ -105,6 +130,19 @@ main(int argc, char **argv)
sentry_options_set_max_spans(options, 5);
}

if (has_arg(argc, argv, "before-send")) {
sentry_options_set_before_send(options, before_send_callback, NULL);
}

if (has_arg(argc, argv, "discarding-on-crash")) {
sentry_options_set_on_crash(
options, discarding_on_crash_callback, NULL);
}

if (has_arg(argc, argv, "retaining-on-crash")) {
sentry_options_set_on_crash(options, retaining_on_crash_callback, NULL);
}

sentry_init(options);

if (!has_arg(argc, argv, "no-setup")) {
Expand Down
11 changes: 9 additions & 2 deletions include/sentry.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ extern "C" {

#include <inttypes.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stddef.h>

/* context type dependencies */
Expand Down Expand Up @@ -771,6 +770,10 @@ SENTRY_API void sentry_options_set_transport(
* call `sentry_value_decref` on the provided event, and return a
* `sentry_value_new_null()` instead.
*
* If you have set an `on_crash` callback (even one that only returns 1),
* `before_send` will no longer be invoked for crash-events. This allows you to
* better differentiate between crashes and all other events.
*
* This function may be invoked inside of a signal handler and must be safe for
* that purpose, see https://man7.org/linux/man-pages/man7/signal-safety.7.html.
* On Windows, it may be called from inside of a `UnhandledExceptionFilter`, see
Expand Down Expand Up @@ -804,13 +807,17 @@ SENTRY_API void sentry_options_set_before_send(
*
* If the callback returns false outgoing crash report will be discarded.
*
* If this callback is set in the options, a concurrently enabled `before_send`
* callback will no longer be called in the crash case. This allows for better
* differentiation between crashes and other events.
*
* This function may be invoked inside of a signal handler and must be safe for
* that purpose, see https://man7.org/linux/man-pages/man7/signal-safety.7.html.
* On Windows, it may be called from inside of a `UnhandledExceptionFilter`, see
* the documentation on SEH (structured exception handling) for more information
* https://docs.microsoft.com/en-us/windows/win32/debug/structured-exception-handling
*/
typedef bool (*sentry_crash_function_t)(
typedef int (*sentry_crash_function_t)(
const sentry_ucontext_t *uctx, void *closure);

/**
Expand Down
4 changes: 2 additions & 2 deletions src/backends/sentry_backend_breakpad.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ sentry__breakpad_backend_callback(

if (should_handle) {
sentry_value_t event = sentry_value_new_event();
sentry_envelope_t *envelope
= sentry__prepare_event(options, event, NULL);
sentry_envelope_t *envelope = sentry__prepare_event(
options, event, nullptr, !options->on_crash_func);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without informing sentry__prepare_event() whether before_send() should be invoked, we cannot make sure that a set on_crash() prevents the execution of the before_send() callback.

// the event we just prepared is empty,
// so no error is recorded for it
sentry__record_errors_on_current_session(1);
Expand Down
4 changes: 2 additions & 2 deletions src/backends/sentry_backend_inproc.c
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,8 @@ handle_ucontext(const sentry_ucontext_t *uctx)
}

if (should_handle) {
supervacuus marked this conversation as resolved.
Show resolved Hide resolved
sentry_envelope_t *envelope
= sentry__prepare_event(options, event, NULL);
sentry_envelope_t *envelope = sentry__prepare_event(
options, event, NULL, !options->on_crash_func);
// TODO(tracing): Revisit when investigating transaction flushing
// during hard crashes.

Expand Down
6 changes: 3 additions & 3 deletions src/sentry_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ sentry__capture_event(sentry_value_t event)
if (sentry__event_is_transaction(event)) {
envelope = sentry__prepare_transaction(options, event, &event_id);
} else {
envelope = sentry__prepare_event(options, event, &event_id);
envelope = sentry__prepare_event(options, event, &event_id, true);
}
if (envelope) {
if (options->session) {
Expand Down Expand Up @@ -460,7 +460,7 @@ sentry__should_send_transaction(sentry_value_t tx_cxt)

sentry_envelope_t *
sentry__prepare_event(const sentry_options_t *options, sentry_value_t event,
sentry_uuid_t *event_id)
sentry_uuid_t *event_id, bool invoke_before_send)
{
sentry_envelope_t *envelope = NULL;

Expand All @@ -477,7 +477,7 @@ sentry__prepare_event(const sentry_options_t *options, sentry_value_t event,
sentry__scope_apply_to_event(scope, options, event, mode);
}

if (options->before_send_func) {
if (options->before_send_func && invoke_before_send) {
SENTRY_TRACE("invoking `before_send` hook");
event
= options->before_send_func(event, NULL, options->before_send_data);
Expand Down
5 changes: 2 additions & 3 deletions src/sentry_core.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,8 @@ bool sentry__event_is_transaction(sentry_value_t event);
* being passed in is not a transaction.
*
* More specifically, it will do the following things:
* - sample the event, possibly discarding it,
* - apply the scope to it,
* - call the before_send hook on it,
* - call the before_send hook on it (if invoke_before_send == true),
* - add the event to a new envelope,
* - record errors on the current session,
* - add any attachments to the envelope as well
Expand All @@ -51,7 +50,7 @@ bool sentry__event_is_transaction(sentry_value_t event);
* `event_id` out-parameter.
*/
sentry_envelope_t *sentry__prepare_event(const sentry_options_t *options,
sentry_value_t event, sentry_uuid_t *event_id);
sentry_value_t event, sentry_uuid_t *event_id, bool invoke_before_send);

/**
* Sends a sentry event, regardless of its type.
Expand Down
23 changes: 21 additions & 2 deletions tests/assertions.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import datetime
import email
import gzip
import sys
import platform
import re
from .conditions import is_android
import sys

from .conditions import is_android

VERSION_RE = re.compile(r"(\d+\.\d+\.\d+)(?:[-\.]?)(.*)")

Expand Down Expand Up @@ -194,6 +194,25 @@ def assert_crash(envelope):
assert_stacktrace(envelope, inside_exception=True, check_size=False)


def assert_crash_timestamp(has_files, tmp_path):
# The crash file should survive a `sentry_init` and should still be there
# even after restarts.
if has_files:
with open("{}/.sentry-native/last_crash".format(tmp_path)) as f:
crash_timestamp = f.read()
assert_timestamp(crash_timestamp)


def assert_before_send(envelope):
event = envelope.get_event()
assert_matches(event, {"adapted_by": "before_send"})


def assert_no_before_send(envelope):
event = envelope.get_event()
assert ("adapted_by", "before_send") not in event.items()


def assert_crashpad_upload(req):
multipart = gzip.decompress(req.get_data())
msg = email.message_from_bytes(bytes(str(req.headers), encoding="utf8") + multipart)
Expand Down