diff --git a/examples/example.c b/examples/example.c index 4d76aeede..ada2e76bf 100644 --- a/examples/example.c +++ b/examples/example.c @@ -246,8 +246,8 @@ main(int argc, char **argv) grandchild, SENTRY_SPAN_STATUS_ALREADY_EXISTS); } - sentry_span_finish(tx, grandchild); - sentry_span_finish(tx, child); + sentry_span_finish(grandchild); + sentry_span_finish(child); } sentry_transaction_finish(tx); diff --git a/include/sentry.h b/include/sentry.h index 9369e8f41..0df2f9c4b 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1276,6 +1276,12 @@ typedef struct sentry_span_s sentry_span_t; * Also see https://develop.sentry.dev/sdk/event-payloads/transaction/#anatomy * for an explanation of `operation`, in addition to other properties and * actions that can be performed on a Transaction. + * + * The returned value is not thread-safe. Users are expected to ensure that + * appropriate locking mechanisms are implemented over the Transaction Context + * if it needs to be mutated across threads. Methods operating on the + * Transaction Context will mention what kind of expectations they carry if they + * need to mutate or access the object in a thread-safe way. */ SENTRY_EXPERIMENTAL_API sentry_transaction_context_t * sentry_transaction_context_new(const char *name, const char *operation); @@ -1283,6 +1289,9 @@ sentry_transaction_context_new(const char *name, const char *operation); /** * Sets the `name` on a Transaction Context, which will be used in the * Transaction constructed off of the context. + * + * The Transaction Context should not be mutated by other functions while + * setting a name on it. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_name( sentry_transaction_context_t *tx_cxt, const char *name); @@ -1293,6 +1302,9 @@ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_name( * * See https://develop.sentry.dev/sdk/performance/span-operations/ for * conventions on `operation`s. + * + * The Transaction Context should not be mutated by other functions while + * setting an operation on it. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_operation( sentry_transaction_context_t *tx_cxt, const char *operation); @@ -1304,15 +1316,21 @@ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_operation( * When passed any value above 0, the Transaction will bypass all sampling * options and always be sent to sentry. If passed 0, this Transaction and its * child spans will never be sent to sentry. + * + * The Transaction Context should not be mutated by other functions while + * setting `sampled` on it. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_sampled( sentry_transaction_context_t *tx_cxt, int sampled); /** - * Removes the sampled field on a Transaction Context, which will be used in the - * Transaction constructed off of the context. + * Removes the `sampled` field on a Transaction Context, which will be used in + * the Transaction constructed off of the context. * * The Transaction will use the sampling rate as defined in `sentry_options`. + * + * The Transaction Context should not be mutated by other functions while + * removing `sampled`. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_remove_sampled( sentry_transaction_context_t *tx_cxt); @@ -1320,21 +1338,29 @@ SENTRY_EXPERIMENTAL_API void sentry_transaction_context_remove_sampled( /** * Starts a new Transaction based on the provided context, restored from an * external integration (i.e. a span from a different SDK) or manually - * constructed by a user. Returns a Transaction, which is expected to be - * manually managed by the caller. Manual management involves ensuring that - * `sentry_transaction_finish` is invoked for the Transaction, and that the - * caller manually starts and finishes any child Spans as needed on the - * Transaction. + * constructed by a user. * - * `sentry_transaction_finish` must be called in order for this Transaction to - * be sent to sentry. + * Returns a Transaction, which is expected to be manually managed by the + * caller. Manual management involves ensuring that `sentry_transaction_finish` + * is invoked for the Transaction, and that the caller manually starts and + * finishes any child Spans as needed on the Transaction. + * + * Not invoking `sentry_transaction_finish` with the returned Transaction means + * it will be discarded, and will not be sent to sentry. * * 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. * - * Takes ownership of `transaction_context`. + * Takes ownership of `transaction_context`. A Transaction Context cannot be + * modified or re-used after it is used to start a Transaction. + * + * The returned value is not thread-safe. Users are expected to ensure that + * appropriate locking mechanisms are implemented over the Transaction if it + * needs to be mutated across threads. Methods operating on the Transaction will + * mention what kind of expectations they carry if they need to mutate or access + * the object in a thread-safe way. */ SENTRY_EXPERIMENTAL_API sentry_transaction_t *sentry_transaction_start( sentry_transaction_context_t *tx_cxt); @@ -1345,8 +1371,8 @@ SENTRY_EXPERIMENTAL_API sentry_transaction_t *sentry_transaction_start( * otherwise. * * Always takes ownership of `transaction`, regardless of whether the operation - * was successful or not. If `sentry_set_span` was invoked with `transaction`, - * this will remove the + * was successful or not. A Transaction cannot be modified or re-used after it + * is finished. */ SENTRY_EXPERIMENTAL_API sentry_uuid_t sentry_transaction_finish( sentry_transaction_t *tx); @@ -1360,10 +1386,10 @@ SENTRY_EXPERIMENTAL_API sentry_uuid_t sentry_transaction_finish( * 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 function. + * 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 function. */ SENTRY_EXPERIMENTAL_API void sentry_set_span(sentry_transaction_t *tx); @@ -1386,6 +1412,14 @@ SENTRY_EXPERIMENTAL_API void sentry_set_span(sentry_transaction_t *tx); * finishing the Span means it will be discarded, and will not be sent to * sentry. `sentry_value_null` will be returned if the child Span could not be * created. + * + * This increases the number of references pointing to the Transaction. + * + * 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 + * kind of expectations they carry if they need to mutate or access the object + * in a thread-safe way. */ SENTRY_EXPERIMENTAL_API sentry_span_t *sentry_transaction_start_child( sentry_transaction_t *parent, char *operation, char *description); @@ -1408,6 +1442,12 @@ SENTRY_EXPERIMENTAL_API sentry_span_t *sentry_transaction_start_child( * finishing the Span means it will be discarded, and will not be sent to * sentry. `sentry_value_null` will be returned if the child Span could not be * created. + * + * 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 + * kind of expectations they carry if they need to mutate or access the object + * in a thread-safe way. */ SENTRY_EXPERIMENTAL_API sentry_span_t *sentry_span_start_child( sentry_span_t *parent, char *operation, char *description); @@ -1415,76 +1455,102 @@ SENTRY_EXPERIMENTAL_API sentry_span_t *sentry_span_start_child( /** * Finishes a Span. * - * `root_transaction` is either the parent Transaction of the Span, or - * the ancestor Transaction of the Span if the Span is not a direct descendant - * of a Transaction. + * This takes ownership of `span`. A Span cannot be modified or re-used after it + * is finished. * - * This takes ownership of `span`, as child Spans must always occur within the - * total duration of a parent Span and cannot outlive their parent Spans. + * This will mutate the `span`'s containing Transaction, so the containing + * Transaction should also not be mutated by other functions when finishing a + * span. */ -SENTRY_EXPERIMENTAL_API void sentry_span_finish( - sentry_transaction_t *root_transaction, sentry_span_t *span); +SENTRY_EXPERIMENTAL_API void sentry_span_finish(sentry_span_t *span); /** - * Sets a tag on a transaction to the given string value. + * Sets a tag on a Transaction to the given string value. * * Tags longer than 200 bytes will be truncated. + * + * The Transaction should not be mutated by other functions while a tag is being + * set on it. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_set_tag( sentry_transaction_t *transaction, const char *tag, const char *value); /** - * Removes a tag from a transaction. + * Removes a tag from a Transaction. + * + * The Transaction should not be mutated by other functions while a tag is being + * removed from it. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_remove_tag( sentry_transaction_t *transaction, const char *tag); /** - * Sets the given key in a transaction's "data" section to the given value. + * Sets the given key in a Transaction's "data" section to the given value. + * + * The Transaction should not be mutated by other functions while data is being + * set on it. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_set_data( sentry_transaction_t *transaction, const char *key, sentry_value_t value); /** - * Removes a key from a transaction's "data" section. + * Removes a key from a Transaction's "data" section. + * + * The Transaction should not be mutated by other functions while data is being + * removed from it. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_remove_data( sentry_transaction_t *transaction, const char *key); /** - * Sets a tag on a span to the given string value. + * Sets a tag on a Span to the given string value. * * Tags longer than 200 bytes will be truncated. + * + * The Span should not be mutated by other functions while a tag is being set on + * it. */ SENTRY_EXPERIMENTAL_API void sentry_span_set_tag( sentry_span_t *span, const char *tag, const char *value); /** - * Removes a tag from a span. + * Removes a tag from a Span. + * + * The Span should not be mutated by other functions while a tag is being + * removed from it. */ SENTRY_EXPERIMENTAL_API void sentry_span_remove_tag( sentry_span_t *span, const char *tag); /** - * Sets the given key in a span's "data" section to the given value. + * Sets the given key in a Span's "data" section to the given value. + * + * The Span should not be mutated by other functions while data is being set on + * it. */ SENTRY_EXPERIMENTAL_API void sentry_span_set_data( sentry_span_t *span, const char *key, sentry_value_t value); /** - * Removes a key from a span's "data" section. + * Removes a key from a Span's "data" section. + * + * The Span should not be mutated by other functions while data is being removed + * from it. */ SENTRY_EXPERIMENTAL_API void sentry_span_remove_data( sentry_span_t *span, const char *key); /** - * Sets a transaction's name. + * Sets a Transaction's name. + * + * The Transaction should not be mutated by other functions while setting its + * name. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_set_name( sentry_transaction_t *transaction, const char *name); /** - * The status of a span or transaction. + * The status of a Span or Transaction. * * See https://develop.sentry.dev/sdk/event-payloads/span/ for documentation. */ @@ -1542,13 +1608,18 @@ typedef enum { } sentry_span_status_t; /** - * Sets a span's status. + * Sets a Span's status. + * + * The Span should not be mutated by other functions while setting its status. */ SENTRY_EXPERIMENTAL_API void sentry_span_set_status( sentry_span_t *span, sentry_span_status_t status); /** - * Sets a transaction's status. + * Sets a Transaction's status. + * + * The Transaction should not be mutated by other functions while setting its + * status. */ SENTRY_EXPERIMENTAL_API void sentry_transaction_set_status( sentry_transaction_t *tx, sentry_span_status_t status); diff --git a/src/sentry_core.c b/src/sentry_core.c index 09a038b1a..17de5a8fe 100644 --- a/src/sentry_core.c +++ b/src/sentry_core.c @@ -864,17 +864,21 @@ sentry_transaction_start_child( max_spans = options->max_spans; } - return sentry__start_child(max_spans, parent, operation, description); - // TODO: add pointer to transaction on child span + sentry_value_t span + = sentry__value_span_new(max_spans, parent, operation, description); + return sentry__span_new(opaque_parent, span); } sentry_span_t * sentry_span_start_child( sentry_span_t *opaque_parent, char *operation, char *description) { - // TODO: check and validate ref to ancestor transaction on parent span if (!opaque_parent || sentry_value_is_null(opaque_parent->inner)) { - SENTRY_DEBUG("no parent available to create a child under"); + SENTRY_DEBUG("no parent span available to create a child span under"); + return NULL; + } + if (!opaque_parent->transaction) { + SENTRY_DEBUG("no root transaction to create a child span under"); return NULL; } sentry_value_t parent = opaque_parent->inner; @@ -886,21 +890,25 @@ sentry_span_start_child( max_spans = options->max_spans; } - return sentry__start_child(max_spans, parent, operation, description); - // TODO: add pointer to ancestor transaction on child span + sentry_value_t span + = sentry__value_span_new(max_spans, parent, operation, description); + + return sentry__span_new(opaque_parent->transaction, span); } -// TODO: don't accept the root transaction as a param, the span should have a -// ref to it already in itself void -sentry_span_finish( - sentry_transaction_t *opaque_root_transaction, sentry_span_t *opaque_span) +sentry_span_finish(sentry_span_t *opaque_span) { - if (!opaque_root_transaction || !opaque_span - || sentry_value_is_null(opaque_root_transaction->inner) - || sentry_value_is_null(opaque_span->inner)) { + if (!opaque_span || sentry_value_is_null(opaque_span->inner)) { + SENTRY_DEBUG("no span to finish"); + goto fail; + } + + sentry_transaction_t *opaque_root_transaction = opaque_span->transaction; + if (!opaque_root_transaction + || sentry_value_is_null(opaque_root_transaction->inner)) { SENTRY_DEBUG( - "missing root transaction or span to finish, aborting span finish"); + "no root transaction to finish span on, aborting span finish"); goto fail; } @@ -921,9 +929,6 @@ sentry_span_finish( goto fail; } - // tough luck if this span actually doesn't belong on the specified - // transaction, i.e. its trace id doesn't match the root transaction's trace - // id sentry_value_set_by_key(span, "timestamp", sentry__value_new_string_owned( sentry__msec_time_to_iso8601(sentry__msec_time()))); diff --git a/src/sentry_tracing.c b/src/sentry_tracing.c index 92d16620b..7eaac5f43 100644 --- a/src/sentry_tracing.c +++ b/src/sentry_tracing.c @@ -161,9 +161,9 @@ sentry__transaction_decref(sentry_transaction_t *tx) } sentry_span_t * -sentry__span_new(sentry_value_t inner) +sentry__span_new(sentry_transaction_t *tx, sentry_value_t inner) { - if (sentry_value_is_null(inner)) { + if (!tx || sentry_value_is_null(inner)) { return NULL; } @@ -175,11 +175,14 @@ sentry__span_new(sentry_value_t inner) span->inner = inner; + sentry__transaction_incref(tx); + span->transaction = tx; + return span; } -sentry_span_t * -sentry__start_child( +sentry_value_t +sentry__value_span_new( size_t max_spans, sentry_value_t parent, char *operation, char *description) { if (!sentry_value_is_null(sentry_value_get_by_key(parent, "timestamp"))) { @@ -195,9 +198,9 @@ sentry__start_child( goto fail; } sentry_value_t spans = sentry_value_get_by_key(parent, "spans"); - // This only checks that the number of _completed_ spans matches the number - // of max spans. This means that the number of in-flight spans can exceed - // the max number of spans. + // This only checks that the number of _completed_ spans matches the + // number of max spans. This means that the number of in-flight spans + // can exceed the max number of spans. if (sentry_value_get_length(spans) >= max_spans) { SENTRY_DEBUG("reached maximum number of spans for transaction, not " "creating span"); @@ -212,14 +215,14 @@ sentry__start_child( sentry__msec_time_to_iso8601(sentry__msec_time()))); sentry_value_set_by_key(child, "sampled", sentry_value_new_bool(1)); - return sentry__span_new(child); + return child; fail: - return NULL; + return sentry_value_new_null(); } // TODO: for now, don't allow multiple references to spans. this should be -// revisited when sentry_transaction_t stores a list of sentry_span_t's instead -// of a list of sentry_value_t's. +// revisited when sentry_transaction_t stores a list of sentry_span_t's +// instead of a list of sentry_value_t's. void sentry__span_free(sentry_span_t *span) { @@ -227,6 +230,7 @@ sentry__span_free(sentry_span_t *span) return; } sentry_value_decref(span->inner); + sentry__transaction_decref(span->transaction); sentry_free(span); } diff --git a/src/sentry_tracing.h b/src/sentry_tracing.h index 47b621f76..c92fd7501 100644 --- a/src/sentry_tracing.h +++ b/src/sentry_tracing.h @@ -9,6 +9,8 @@ */ typedef struct sentry_span_s { sentry_value_t inner; + // The transaction the span is contained in. + sentry_transaction_t *transaction; } sentry_span_t; /** @@ -31,8 +33,10 @@ 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); -sentry_span_t *sentry__start_child(size_t max_spans, sentry_value_t parent, +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( + sentry_transaction_t *parent_tx, sentry_value_t inner); void sentry__span_free(sentry_span_t *span); /** diff --git a/tests/unit/test_tracing.c b/tests/unit/test_tracing.c index 85cbdbd1b..f37089785 100644 --- a/tests/unit/test_tracing.c +++ b/tests/unit/test_tracing.c @@ -372,7 +372,7 @@ SENTRY_TEST(basic_spans) // Sanity check that child isn't finished yet TEST_CHECK(IS_NULL(child, "timestamp")); // Now finishing - sentry_span_finish(opaque_tx, opaque_child); + sentry_span_finish(opaque_child); TEST_CHECK(!IS_NULL(tx, "spans")); sentry_value_t spans = sentry_value_get_by_key(tx, "spans"); @@ -422,7 +422,7 @@ SENTRY_TEST(spans_on_scope) // Sanity check that child isn't finished yet TEST_CHECK(IS_NULL(child, "timestamp")); - sentry_span_finish(opaque_tx, opaque_child); + sentry_span_finish(opaque_child); scope_tx = sentry__scope_get_span(); TEST_CHECK(!IS_NULL(scope_tx, "spans")); @@ -469,7 +469,7 @@ SENTRY_TEST(child_spans) // Shouldn't be added to spans yet TEST_CHECK(IS_NULL(tx, "spans")); - sentry_span_finish(opaque_tx, opaque_grandchild); + sentry_span_finish(opaque_grandchild); // Make sure everything on the transaction looks good, check grandchild const char *trace_id @@ -489,7 +489,7 @@ SENTRY_TEST(child_spans) // Should be finished TEST_CHECK(!IS_NULL(stored_grandchild, "timestamp")); - sentry_span_finish(opaque_tx, opaque_child); + sentry_span_finish(opaque_child); spans = sentry_value_get_by_key(tx, "spans"); TEST_CHECK_INT_EQUAL(sentry_value_get_length(spans), 2); @@ -526,7 +526,7 @@ SENTRY_TEST(overflow_spans) // Shouldn't be added to spans yet TEST_CHECK(IS_NULL(tx, "spans")); - sentry_span_finish(opaque_tx, opaque_child); + sentry_span_finish(opaque_child); TEST_CHECK(!IS_NULL(tx, "spans")); sentry_value_t spans = sentry_value_get_by_key(tx, "spans"); @@ -535,7 +535,7 @@ SENTRY_TEST(overflow_spans) sentry_value_t stored_child = sentry_value_get_by_index(spans, 0); CHECK_STRING_PROPERTY(stored_child, "span_id", child_span_id); - sentry_span_finish(opaque_tx, opaque_drop_on_finish_child); + sentry_span_finish(opaque_drop_on_finish_child); TEST_CHECK_INT_EQUAL(sentry_value_get_length(spans), 1); sentry_span_t *opaque_drop_on_start_child @@ -548,60 +548,6 @@ SENTRY_TEST(overflow_spans) sentry_close(); } -SENTRY_TEST(wrong_spans_on_transaction_is_ok) -{ - sentry_options_t *options = sentry_options_new(); - sentry_options_set_traces_sample_rate(options, 1.0); - sentry_options_set_max_spans(options, 5); - sentry_init(options); - - sentry_transaction_context_t *opaque_tx_cxt - = sentry_transaction_context_new("wow!", NULL); - sentry_transaction_t *opaque_tx = sentry_transaction_start(opaque_tx_cxt); - sentry_value_t tx = opaque_tx->inner; - - sentry_span_t *opaque_child - = sentry_transaction_start_child(opaque_tx, "honk", "goose"); - sentry_value_t child = opaque_child->inner; - const char *child_span_id - = sentry_value_as_string(sentry_value_get_by_key(child, "span_id")); - - sentry_span_t *opaque_lingering_child - = sentry_transaction_start_child(opaque_tx, "beep", "car"); - - sentry_transaction_context_t *tx_cxt_other - = sentry_transaction_context_new("whoa!", NULL); - sentry_transaction_t *opaque_tx_other - = sentry_transaction_start(tx_cxt_other); - sentry_value_t tx_other = opaque_tx_other->inner; - - sentry_span_finish(opaque_tx_other, opaque_child); - - // doesn't care if the child was finished on the wrong transaction - TEST_CHECK(IS_NULL(tx, "spans")); - TEST_CHECK(!IS_NULL(tx_other, "spans")); - - sentry_value_t spans = sentry_value_get_by_key(tx_other, "spans"); - TEST_CHECK_INT_EQUAL(sentry_value_get_length(spans), 1); - - sentry_value_t stored_child = sentry_value_get_by_index(spans, 0); - CHECK_STRING_PROPERTY(stored_child, "span_id", child_span_id); - - sentry_uuid_t event_id = sentry_transaction_finish(opaque_tx); - TEST_CHECK(!sentry_uuid_is_nil(&event_id)); - - // doesn't care if the child belonged to a different, already finished - // transaction - sentry_span_finish(opaque_tx_other, opaque_lingering_child); - TEST_CHECK(!IS_NULL(tx_other, "spans")); - spans = sentry_value_get_by_key(tx_other, "spans"); - TEST_CHECK_INT_EQUAL(sentry_value_get_length(spans), 2); - - sentry__transaction_decref(opaque_tx_other); - - sentry_close(); -} - static void check_spans(sentry_envelope_t *envelope, void *data) { @@ -647,7 +593,7 @@ SENTRY_TEST(drop_unfinished_spans) = sentry_span_start_child(opaque_child, "beep", "car"); sentry_value_t grandchild = opaque_grandchild->inner; TEST_CHECK(!sentry_value_is_null(grandchild)); - sentry_span_finish(opaque_tx, opaque_grandchild); + sentry_span_finish(opaque_grandchild); // spans are only added to transactions upon completion TEST_CHECK_INT_EQUAL( @@ -656,7 +602,8 @@ SENTRY_TEST(drop_unfinished_spans) sentry_uuid_t event_id = sentry_transaction_finish(opaque_tx); TEST_CHECK(!sentry_uuid_is_nil(&event_id)); - sentry__span_free(opaque_child); + // check that nothing explodes if you do finish the lingering child + sentry_span_finish(opaque_child); sentry_close(); diff --git a/tests/unit/tests.inc b/tests/unit/tests.inc index 2341c3358..3460600ae 100644 --- a/tests/unit/tests.inc +++ b/tests/unit/tests.inc @@ -84,4 +84,3 @@ XX(value_object_merge_nested) XX(value_string) XX(value_unicode) XX(value_wrong_type) -XX(wrong_spans_on_transaction_is_ok)