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.

Sign up for GitHub

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

Jump to bottom

feat: add emitting of transaction data inside context trace data #1075

Merged
merged 10 commits into from
Nov 14, 2024
Merged

feat: add emitting of transaction data inside context trace data #1075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1ba43cc
add emitting of transaction data inside context trace data
JoshuaMoelans Nov 6, 2024
30ab79c
format
JoshuaMoelans Nov 6, 2024
4894aaa
fix test name
JoshuaMoelans Nov 6, 2024
a6eafed
update test + proper data store
JoshuaMoelans Nov 7, 2024
c8ba4a8
remove unnecessary test
JoshuaMoelans Nov 7, 2024
0e63492
remove unnecessary test from tests.inc
JoshuaMoelans Nov 7, 2024
42c8114
update CHANGELOG.md
JoshuaMoelans Nov 7, 2024
0623fe6
renamed transaction.extra -> transaction.data
JoshuaMoelans Nov 7, 2024
65c4373
apply scoped transaction/span data to every in-scope event
JoshuaMoelans Nov 7, 2024
3997178
Merge branch 'master' into feat/emit-transaction-data-in-trace-contex…
supervacuus Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
**Fixes**:

- Correct the timeout specified for the upload-task awaiting `dispatch_semaphore_wait()` when using an HTTP-proxy on macOS. ([#1077](https://github.com/getsentry/sentry-native/pull/1077), [crashpad#111](https://github.com/getsentry/crashpad/pull/111))
- Emit `transaction.data` inside `context.trace.data`. ([#1075](https://github.com/getsentry/sentry-native/pull/1075))

**Thank you**:

Expand Down
4 changes: 4 additions & 0 deletions src/sentry_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,6 +934,9 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx)
sentry_value_t trace_context
= sentry__value_get_trace_context(opaque_tx->inner);
sentry_value_t contexts = sentry_value_new_object();
sentry_value_set_by_key(
trace_context, "data", sentry_value_get_by_key(tx, "data"));
sentry_value_incref(sentry_value_get_by_key(tx, "data"));
Comment on lines +937 to +939
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is one missing piece left (added a todo to the PR description): we should apply the currently scoped transaction/span "data" to every event sent inside that transaction/span here:

sentry-native/src/sentry_scope.c

Lines 321 to 328 in ffdf6ed

sentry_value_t scope_trace = sentry__value_get_trace_context(
sentry__get_span_or_transaction(scope));
if (!sentry_value_is_null(scope_trace)) {
if (sentry_value_is_null(contexts)) {
contexts = sentry_value_new_object();
}
sentry_value_set_by_key(contexts, "trace", scope_trace);
}

If the scope_trace isn't a null object, we can also read the "data" item of that scoped transaction/span and place it in the events trace context. Again, you must use incref here because it will be decrefed when sending the envelope, meaning it would be freed and no longer accessible to the transaction/span.

Removal is unnecessary since the transaction/span will continue to exist beyond the lifetime of the event.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The data item should only be added to the event if it isn't a null object in the scoped transaction/span. It doesn't make a difference in the backend (null object == non-existent key, most of the time), but there is no need to send data over the wire that signifies non-existent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a correct implementation of what you asked for? (i.e. when a transaction object exists inside the scope, we get its "data" key item and put it at the scope_trace "data" key)

sentry_value_t scope_trace = sentry__value_get_trace_context(
    sentry__get_span_or_transaction(scope));
if (!sentry_value_is_null(scope_trace)) {
    if (sentry_value_is_null(contexts)) {
        contexts = sentry_value_new_object();
    }
    if (scope->transaction_object) {
        sentry_value_set_by_key(scope_trace, "data",
            sentry_value_get_by_key(
                scope->transaction_object->inner, "data"));
        sentry_value_incref(sentry_value_get_by_key(
            scope->transaction_object->inner, "data"));
    }
    sentry_value_set_by_key(contexts, "trace", scope_trace);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use either span or transaction as returned from sentry__get_span_or_transaction(scope) and don't have to retrieve the transaction specifically. Both can have data items, and they should apply theirs to the event. If, for instance, a span should inherit its data from the transaction, then that should happen at span creation.

@markushi, do you also apply data when you apply the trace context to events from your scope transaction/spans? If so, do you take it solely from the transaction or also from spans?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, I would probably do something like this:

sentry_value_t scoped_txn_or_span = sentry__get_span_or_transaction(scope)
sentry_value_t scope_trace = sentry__value_get_trace_context(scoped_txn_or_span);
if (!sentry_value_is_null(scope_trace)) {
    if (sentry_value_is_null(contexts)) {
        contexts = sentry_value_new_object();
    }
    sentry_value_t scoped_txn_or_span_data = sentry_value_get_by_key(scoped_txn_or_span, "data");
    if (!sentry_value_is_null(scoped_txn_or_span_data)) {
        sentry_value_incref(scoped_txn_or_span_data);
        sentry_value_set_by_key(scope_trace, "data", scoped_txn_or_span_data);
    }
    sentry_value_set_by_key(contexts, "trace", scope_trace);
}

Copy link
Member

@markushi markushi Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@supervacuus @JoshuaMoelans Yes, we do the same but we solely take the context from the transaction:
https://github.com/getsentry/sentry-java/blob/28a11a7925b8e45bec38684f2002192787f13477/sentry/src/main/java/io/sentry/SentryClient.java#L216-L229

It's worth mentioning that at least in sentry-java a span doesn't really have it's own trace context, it simply returns the top level txn context (see https://github.com/getsentry/sentry-java/blob/2e90ac7733c6c9b941e7c64175f3d097d1d15a37/sentry/src/main/java/io/sentry/Span.java#L170)

sentry_value_set_by_key(contexts, "trace", trace_context);
sentry_value_set_by_key(tx, "contexts", contexts);

Expand All @@ -944,6 +947,7 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx)
sentry_value_remove_by_key(tx, "op");
sentry_value_remove_by_key(tx, "description");
sentry_value_remove_by_key(tx, "status");
sentry_value_remove_by_key(tx, "data");

sentry__transaction_decref(opaque_tx);

Expand Down
12 changes: 10 additions & 2 deletions src/sentry_scope.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,12 +318,20 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope,

// prep contexts sourced from scope; data about transaction on scope needs
// to be extracted and inserted
sentry_value_t scope_trace = sentry__value_get_trace_context(
sentry__get_span_or_transaction(scope));
sentry_value_t scoped_txn_or_span = sentry__get_span_or_transaction(scope);
sentry_value_t scope_trace
= sentry__value_get_trace_context(scoped_txn_or_span);
if (!sentry_value_is_null(scope_trace)) {
if (sentry_value_is_null(contexts)) {
contexts = sentry_value_new_object();
}
sentry_value_t scoped_txn_or_span_data
= sentry_value_get_by_key(scoped_txn_or_span, "data");
if (!sentry_value_is_null(scoped_txn_or_span_data)) {
sentry_value_incref(scoped_txn_or_span_data);
sentry_value_set_by_key(
scope_trace, "data", scoped_txn_or_span_data);
}
sentry_value_set_by_key(contexts, "trace", scope_trace);
}

Expand Down
2 changes: 1 addition & 1 deletion src/sentry_tracing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ sentry_transaction_set_data(
}
}

static const char txn_data_key[] = "extra";
static const char txn_data_key[] = "data";
static const size_t txn_data_key_len = sizeof(txn_data_key) - 1;

void
Expand Down
3 changes: 2 additions & 1 deletion tests/test_integration_http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,6 @@ def test_transaction_only(cmake, httpserver, build_args):
assert_meta(
envelope,
transaction="little.teapot",
transaction_data={"url": "https://example.com"},
)

# Extract the one-and-only-item
Expand Down Expand Up @@ -575,6 +574,8 @@ def test_transaction_only(cmake, httpserver, build_args):
timestamp = time.strptime(payload["timestamp"], RFC3339_FORMAT)
assert timestamp >= start_timestamp

assert trace_context["data"] == {"url": "https://example.com"}
JoshuaMoelans marked this conversation as resolved.
Show resolved Hide resolved


def test_capture_minidump(cmake, httpserver):
tmp_path = cmake(["sentry_example"], {"SENTRY_BACKEND": "none"})
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/test_tracing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,10 +1089,10 @@ SENTRY_TEST(txn_data)

sentry_transaction_set_data(
txn, "os.name", sentry_value_new_string("Linux"));
check_after_set(txn->inner, "extra", "os.name", "Linux");
check_after_set(txn->inner, "data", "os.name", "Linux");

sentry_transaction_remove_data(txn, "os.name");
check_after_remove(txn->inner, "extra", "os.name");
check_after_remove(txn->inner, "data", "os.name");

sentry__transaction_decref(txn);
}
Expand Down Expand Up @@ -1139,10 +1139,10 @@ SENTRY_TEST(txn_data_n)
sentry_value_t data_value
= sentry_value_new_string_n(data_v, sizeof(data_v));
sentry_transaction_set_data_n(txn, data_k, sizeof(data_k), data_value);
check_after_set(txn->inner, "extra", "os.name", "Linux");
check_after_set(txn->inner, "data", "os.name", "Linux");

sentry_transaction_remove_data_n(txn, data_k, sizeof(data_k));
check_after_remove(txn->inner, "extra", "os.name");
check_after_remove(txn->inner, "data", "os.name");

sentry__transaction_decref(txn);
}
Expand Down
Loading