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

feat(tracing): Allow setting a scope on a span #667

Merged
merged 8 commits into from
Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 29 additions & 7 deletions include/sentry.h
Original file line number Diff line number Diff line change
Expand Up @@ -1366,8 +1366,8 @@ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_update_from_header(
*
* To ensure that any Events or Message Events are associated with this
* Transaction while it is active, invoke and pass in the Transaction returned
* by this function to `sentry_set_span`. Further documentation on this can be
* found in `sentry_set_span`'s docstring.
* by this function to `sentry_set_transaction_object`. Further documentation on
* this can be found in `sentry_set_transaction_object`'s docstring.
*
* Takes ownership of `transaction_context`. A Transaction Context cannot be
* modified or re-used after it is used to start a Transaction.
Expand All @@ -1394,20 +1394,32 @@ SENTRY_EXPERIMENTAL_API sentry_uuid_t sentry_transaction_finish(
sentry_transaction_t *tx);

/**
* Sets the Span (actually Transaction) so any Events sent while the Transaction
* Sets the Transaction so any Events sent while the Transaction
* is active will be associated with the Transaction.
*
* If the Transaction being passed in is unsampled, it will still be associated
* with any new Events. This will lead to some Events pointing to orphan or
* missing traces in sentry, see
* https://docs.sentry.io/product/sentry-basics/tracing/trace-view/#orphan-traces-and-broken-subtraces
*
* This increases the number of references pointing to the transaction. Invoke
* `sentry_transaction_finish` to remove the Span set by this function as well
* as its reference by passing in the same Transaction as the one passed into
* This increases the number of references pointing to the Transaction. Invoke
* `sentry_transaction_finish` to remove the Transaction set by this function as
* well as its reference by passing in the same Transaction as the one passed
* into this function.
*/
SENTRY_EXPERIMENTAL_API void sentry_set_transaction_object(
sentry_transaction_t *tx);

/**
* Sets the Span so any Events sent while the Span
* is active will be associated with the Span.
*
* This increases the number of references pointing to the Span. Invoke
* `sentry_span_finish` to remove the Span set by this function as well
* as its reference by passing in the same Span as the one passed into
* this function.
*/
SENTRY_EXPERIMENTAL_API void sentry_set_span(sentry_transaction_t *tx);
SENTRY_EXPERIMENTAL_API void sentry_set_span(sentry_span_t *span);

/**
* Starts a new Span.
Expand All @@ -1429,6 +1441,11 @@ SENTRY_EXPERIMENTAL_API void sentry_set_span(sentry_transaction_t *tx);
* sentry. `sentry_value_null` will be returned if the child Span could not be
* created.
*
* To ensure that any Events or Message Events are associated with this
* Span while it is active, invoke and pass in the Span returned
* by this function to `sentry_set_span`. Further documentation on this can be
* found in `sentry_set_span`'s docstring.
*
* This increases the number of references pointing to the Transaction.
*
* The returned value is not thread-safe. Users are expected to ensure that
Expand Down Expand Up @@ -1459,6 +1476,11 @@ SENTRY_EXPERIMENTAL_API sentry_span_t *sentry_transaction_start_child(
* sentry. `sentry_value_null` will be returned if the child Span could not be
* created.
*
* To ensure that any Events or Message Events are associated with this
* Span while it is active, invoke and pass in the Span returned
* by this function to `sentry_set_span`. Further documentation on this can be
* found in `sentry_set_span`'s docstring.
*
* The returned value is not thread-safe. Users are expected to ensure that
* appropriate locking mechanisms are implemented over the Span if it needs
* to be mutated across threads. Methods operating on the Span will mention what
Expand Down
49 changes: 39 additions & 10 deletions src/sentry_core.c
Original file line number Diff line number Diff line change
Expand Up @@ -714,8 +714,8 @@ sentry_set_transaction(const char *transaction)
scope->transaction = sentry__string_clone(transaction);

#ifdef SENTRY_PERFORMANCE_MONITORING
if (scope->span) {
sentry_transaction_set_name(scope->span, transaction);
if (scope->transaction_object) {
sentry_transaction_set_name(scope->transaction_object, transaction);
}
#endif
}
Expand Down Expand Up @@ -778,16 +778,16 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx)
sentry_value_t tx = sentry__value_clone(opaque_tx->inner);

SENTRY_WITH_SCOPE_MUT (scope) {
if (scope->span) {
sentry_value_t scope_tx = scope->span->inner;
if (scope->transaction_object) {
sentry_value_t scope_tx = scope->transaction_object->inner;

const char *tx_id = sentry_value_as_string(
sentry_value_get_by_key(tx, "trace_id"));
const char *scope_tx_id = sentry_value_as_string(
sentry_value_get_by_key(scope_tx, "trace_id"));
if (sentry__string_eq(tx_id, scope_tx_id)) {
sentry__transaction_decref(scope->span);
scope->span = NULL;
sentry__transaction_decref(scope->transaction_object);
scope->transaction_object = NULL;
}
}
}
Expand Down Expand Up @@ -819,7 +819,7 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx)

// TODO: add tracestate
sentry_value_t trace_context
= sentry__transaction_get_trace_context(opaque_tx);
= sentry__value_get_trace_context(opaque_tx->inner);
sentry_value_t contexts = sentry_value_new_object();
sentry_value_set_by_key(contexts, "trace", trace_context);
sentry_value_set_by_key(tx, "contexts", contexts);
Expand All @@ -843,12 +843,26 @@ sentry_transaction_finish(sentry_transaction_t *opaque_tx)
}

void
sentry_set_span(sentry_transaction_t *tx)
sentry_set_transaction_object(sentry_transaction_t *tx)
{
SENTRY_WITH_SCOPE_MUT (scope) {
sentry__transaction_decref(scope->span);
sentry__span_decref(scope->span);
scope->span = NULL;
sentry__transaction_decref(scope->transaction_object);
sentry__transaction_incref(tx);
scope->span = tx;
scope->transaction_object = tx;
}
}

void
sentry_set_span(sentry_span_t *span)
{
SENTRY_WITH_SCOPE_MUT (scope) {
sentry__transaction_decref(scope->transaction_object);
scope->transaction_object = NULL;
sentry__span_decref(scope->span);
sentry__span_incref(span);
scope->span = span;
}
}

Expand Down Expand Up @@ -928,6 +942,21 @@ sentry_span_finish(sentry_span_t *opaque_span)

sentry_value_t span = sentry__value_clone(opaque_span->inner);

SENTRY_WITH_SCOPE_MUT (scope) {
if (scope->span) {
sentry_value_t scope_span = scope->span->inner;

const char *span_id = sentry_value_as_string(
sentry_value_get_by_key(span, "trace_id"));
const char *scope_span_id = sentry_value_as_string(
sentry_value_get_by_key(scope_span, "trace_id"));
if (sentry__string_eq(span_id, scope_span_id)) {
sentry__span_decref(scope->span);
scope->span = NULL;
}
}
}

if (!sentry_value_is_null(sentry_value_get_by_key(span, "timestamp"))) {
SENTRY_DEBUG("span is already finished, aborting span finish");
sentry_value_decref(span);
Expand Down
28 changes: 19 additions & 9 deletions src/sentry_scope.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ get_scope(void)
g_scope.client_sdk = get_client_sdk();

#ifdef SENTRY_PERFORMANCE_MONITORING
g_scope.transaction_object = NULL;
g_scope.span = NULL;
#endif

Expand All @@ -106,7 +107,8 @@ sentry__scope_cleanup(void)
sentry_value_decref(g_scope.client_sdk);

#ifdef SENTRY_PERFORMANCE_MONITORING
sentry__transaction_decref(g_scope.span);
sentry__transaction_decref(g_scope.transaction_object);
sentry__span_decref(g_scope.span);
#endif
}
sentry__mutex_unlock(&g_lock);
Expand Down Expand Up @@ -240,16 +242,24 @@ sentry__symbolize_stacktrace(sentry_value_t stacktrace)
}

#ifdef SENTRY_PERFORMANCE_MONITORING
sentry_value_t
sentry__get_span_or_transaction(const sentry_scope_t *scope)
{
if (scope->span) {
return scope->span->inner;
} else if (scope->transaction_object) {
return scope->transaction_object->inner;
} else {
return sentry_value_new_null();
}
}

# ifdef SENTRY_UNITTEST
sentry_value_t
sentry__scope_get_span()
sentry__scope_get_span_or_transaction()
{
SENTRY_WITH_SCOPE (scope) {
if (!scope->span) {
return sentry_value_new_null();
} else {
return scope->span->inner;
}
return sentry__get_span_or_transaction(scope);
}
return sentry_value_new_null();
}
Expand Down Expand Up @@ -320,8 +330,8 @@ sentry__scope_apply_to_event(const sentry_scope_t *scope,
sentry_value_t contexts = sentry__value_clone(scope->contexts);
// prep contexts sourced from scope; data about transaction on scope needs
// to be extracted and inserted
sentry_value_t scope_trace
= sentry__transaction_get_trace_context(scope->span);
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();
Expand Down
18 changes: 11 additions & 7 deletions src/sentry_scope.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,16 @@ typedef struct sentry_scope_s {
sentry_value_t client_sdk;

#ifdef SENTRY_PERFORMANCE_MONITORING
// Not to be confused with transaction, which is a legacy value. This is
// also known as a transaction, but to maintain consistency with other SDKs
// and to avoid a conflict with the existing transaction field this is named
// span. Whenever possible, `transaction` should pull its value from the
// `name` property nested in this field.
sentry_transaction_t *span;
// The span attached to this scope, if any.
//
// Conceptually, every transaction is a span, so it should be possible to
// attach spans or transactions to a scope. But sentry_span_t and
// sentry_transaction_t are unrelated types in the native SDK, so we need
// two distinct pointers. At most one of them should ever be non-null.
// Whenever possible, `transaction` should pull its value from the
// `name` property nested in transaction_object or span.
sentry_transaction_t *transaction_object;
sentry_span_t *span;
#endif
} sentry_scope_t;

Expand Down Expand Up @@ -97,6 +101,6 @@ void sentry__scope_apply_to_event(const sentry_scope_t *scope,
#ifdef SENTRY_PERFORMANCE_MONITORING
// this is only used in unit tests
#ifdef SENTRY_UNITTEST
sentry_value_t sentry__scope_get_span();
sentry_value_t sentry__scope_get_span_or_transaction();
#endif
#endif
42 changes: 31 additions & 11 deletions src/sentry_tracing.c
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,27 @@ sentry__transaction_decref(sentry_transaction_t *tx)
};
}

void
sentry__span_incref(sentry_span_t *span)
{
sentry_value_incref(span->inner);
}

void
sentry__span_decref(sentry_span_t *span)
{
if (!span) {
return;
}

if (sentry_value_refcount(span->inner) <= 1) {
sentry_value_decref(span->inner);
sentry_free(span);
Comment on lines +221 to +223
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 I find it quite interesting to piggyback on the inner values refcount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wasn't me, these functions are literally copy-pasted from the corresponding functions for transactions ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

This was a quick and easy way to staple refcounting onto these types without going the full mile, but it could be wrong in some way; I'll admit that I'm not too good at this C thing. Could there be subtle issues with this approach?

For example, there's a small issue with the above ref counting implementation lead to the creation of sentry__span_free (L268-280 in the original, or L289-301 in this PR) instead of sentry__span_incref and sentry__span_decref: if we pass ownership of the inner sentry_value_t to somebody else, sentry__span_decref is unable to determine on its own whether it is supposed to free the wrapping span.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having thought about it a little, I think I have an idea on how to deal with a very specific instance of the example that exists in the SDK, but the general problem is still there.

I'll see what I can do to implement the fix.

} else {
sentry_value_decref(span->inner);
};
}

sentry_span_t *
sentry__span_new(sentry_transaction_t *tx, sentry_value_t inner)
{
Expand Down Expand Up @@ -280,15 +301,14 @@ sentry__span_free(sentry_span_t *span)
}

sentry_value_t
sentry__transaction_get_trace_context(sentry_transaction_t *opaque_tx)
sentry__value_get_trace_context(sentry_value_t span)
{
if (!opaque_tx || sentry_value_is_null(opaque_tx->inner)) {
if (sentry_value_is_null(span)) {
return sentry_value_new_null();
}

sentry_value_t tx = opaque_tx->inner;
if (sentry_value_is_null(sentry_value_get_by_key(tx, "trace_id"))
|| sentry_value_is_null(sentry_value_get_by_key(tx, "span_id"))) {
if (sentry_value_is_null(sentry_value_get_by_key(span, "trace_id"))
|| sentry_value_is_null(sentry_value_get_by_key(span, "span_id"))) {
return sentry_value_new_null();
}

Expand All @@ -303,12 +323,12 @@ sentry__transaction_get_trace_context(sentry_transaction_t *opaque_tx)
} \
} while (0)

PLACE_VALUE("trace_id", tx);
PLACE_VALUE("span_id", tx);
PLACE_VALUE("parent_span_id", tx);
PLACE_VALUE("op", tx);
PLACE_VALUE("description", tx);
PLACE_VALUE("status", tx);
PLACE_VALUE("trace_id", span);
PLACE_VALUE("span_id", span);
PLACE_VALUE("parent_span_id", span);
PLACE_VALUE("op", span);
PLACE_VALUE("description", span);
PLACE_VALUE("status", span);

// TODO: freeze this
return trace_context;
Expand Down
8 changes: 5 additions & 3 deletions src/sentry_tracing.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ sentry_transaction_t *sentry__transaction_new(sentry_value_t inner);
void sentry__transaction_incref(sentry_transaction_t *tx);
void sentry__transaction_decref(sentry_transaction_t *tx);

void sentry__span_incref(sentry_span_t *span);
void sentry__span_decref(sentry_span_t *span);

sentry_value_t sentry__value_span_new(size_t max_spans, sentry_value_t parent,
char *operation, char *description);
sentry_span_t *sentry__span_new(
Expand All @@ -41,10 +44,9 @@ void sentry__span_free(sentry_span_t *span);

/**
* Returns an object containing tracing information extracted from a
* transaction (/span) which should be included in an event.
* transaction / span which should be included in an event.
* See https://develop.sentry.dev/sdk/event-payloads/transaction/#examples
*/
sentry_value_t sentry__transaction_get_trace_context(
sentry_transaction_t *span);
sentry_value_t sentry__value_get_trace_context(sentry_value_t span);

#endif
Loading