feat: add emitting of transaction data inside context trace data

[JoshuaMoelans]

fixes #1074
related to getsentry/team-sdks#95

ToDo:

• emit transaction data through context trace data
• emit transaction data through context trace data in any event being sent during a transaction
• add/update proper test for this
• add to changelog

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 3997178

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.55%. Comparing base (34a9901) to head (3997178).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1075      +/-   ##
==========================================
- Coverage   82.67%   82.55%   -0.12%     
==========================================
  Files          53       53              
  Lines        7717     7729      +12     
  Branches     1213     1214       +1     
==========================================
+ Hits         6380     6381       +1     
- Misses       1225     1235      +10     
- Partials      112      113       +1     

[supervacuus]

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.

[supervacuus]

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.

[JoshuaMoelans]

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);
}

[supervacuus]

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?

[supervacuus]

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);
}

[markushi]

@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)

[supervacuus]

We can merge this because transaction data is now handled as expected.

However this:

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)

is worth further inspection because we deviate quite a bit, and the current implementation of how the trace context is created (and maintained over transaction and span life cycles is insufficient).

I had a deeper look into our implementation and the one in sentry-java. I could imagine anything from only fixing the retrieval function (to choose either the scoped transaction or the transaction linked to from the scoped span) to a more involved rewrite of how we carry around and maintain the trace context. Either way, this won't happen in this PR.

CC: @markushi, @kahest