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

feat(tracing): Defer some transaction validation and allow creation of internal spans #633

Merged
merged 2 commits into from
Dec 23, 2021
Merged

feat(tracing): Defer some transaction validation and allow creation of internal spans #633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2721309
feat(tracing): Defer some transaction validation and allow creation o…
relaxolotl Dec 17, 2021
d27e0a5
oops, double set on operation
relaxolotl Dec 23, 2021
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
19 changes: 15 additions & 4 deletions src/sentry_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,13 @@ sentry_set_level(sentry_level_t level)
sentry_value_t
sentry_transaction_start(sentry_value_t tx_cxt)
{
// TODO: it would be nice if we could just merge tx_cxt into tx.
// `sentry_value_new_transaction_event()` is also an option, but risks
// causing more confusion as there's already a
// `sentry_value_new_transaction`. The ending timestamp is stripped as well
Comment on lines +716 to +719
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you're thinking of aiming for here. If it is purely about the internal representation of the tx_ctx object in the tx object then I'm not sure what would improve. Also the current public API seems fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

being able to invoke some sort of sentry_value_merge_objects(tx, tx_cxt) where tx_cxt wins out would be much more preferable to us manually enumerating over every tx_cxt field, checking for its existence, and stuffing the value into tx if it's present in tx_cxt. that's what this means by "just merging tx_cxt into tx". the function just wants a union of those two objects.

// to avoid misleading ourselves later down the line.
sentry_value_t tx = sentry_value_new_event();
sentry_value_remove_by_key(tx, "timestamp");

// TODO(tracing): stuff transaction into the scope
bool should_sample = sentry__should_send_transaction(tx_cxt);
Expand All @@ -733,8 +739,10 @@ sentry_transaction_start(sentry_value_t tx_cxt)
tx, "trace_id", sentry_value_get_by_key_owned(tx_cxt, "trace_id"));
sentry_value_set_by_key(
tx, "span_id", sentry_value_get_by_key_owned(tx_cxt, "trace_id"));
sentry_value_set_by_key(tx, "transaction",
sentry_value_get_by_key_owned(tx_cxt, "transaction"));
sentry_value_set_by_key(
tx, "transaction", sentry_value_get_by_key_owned(tx_cxt, "name"));
tx, "status", sentry_value_get_by_key_owned(tx_cxt, "status"));
sentry_value_set_by_key(tx, "start_timestamp",
sentry__value_new_string_owned(
sentry__msec_time_to_iso8601(sentry__msec_time())));
Expand Down Expand Up @@ -765,10 +773,13 @@ sentry_transaction_finish(sentry_value_t tx)
sentry__msec_time_to_iso8601(sentry__msec_time())));
sentry_value_set_by_key(tx, "level", sentry_value_new_string("info"));

// TODO(tracing): add tracestate
// set up trace context so it mirrors the final json value
sentry_value_set_by_key(tx, "status", sentry_value_new_string("ok"));
sentry_value_t name = sentry_value_get_by_key(tx, "transaction");
if (sentry_value_is_null(name) || sentry_value_get_length(name) == 0) {
sentry_value_set_by_key(tx, "transaction",
sentry_value_new_string("<unlabeled transaction>"));
}

// TODO: add tracestate
sentry_value_t trace_context = sentry__span_get_trace_context(tx);
sentry_value_t contexts = sentry_value_new_object();
sentry_value_set_by_key(contexts, "trace", trace_context);
Expand Down
35 changes: 25 additions & 10 deletions src/sentry_value.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,10 +1124,32 @@ sentry_value_new_stacktrace(void **ips, size_t len)
return stacktrace;
}

sentry_value_t
sentry__value_new_span(sentry_value_t parent, const char *operation)
{
sentry_value_t span = sentry_value_new_object();

sentry_transaction_context_set_operation(span, operation);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should now be renamed sentry_span_set_operation(span, operation). Setting the operation on the transaction context is already a very nice API because you might as well pass the right value into the sentry_value_new_transaction_context() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think what we can do is define one common implementation for setting operations on spans/transactions/transaction contexts, but i'm thinking that the final API will most likely include a sentry_span_set_operation(...), sentry_transaction_set_operation(...), and sentry_transaction_context_set_operation(...).

i think having a setter regardless can be useful in case one is only able to figure out what a transaction might be after a transaction context is created, or a transaction is started. it's not an immutable field until the transaction is finished.

regarding the naming and reusing ..._span_[operation] for transactions and transaction contexts, i can't say that i'm too big of a fan of relying on the implicit knowledge that transactions are spans with extra bells and whistles in this SDK, since:

  1. there's no such thing as "inheritance" in this to begin with
  2. nothing in the types or the codebase hint at the idea that a transaction "is" a span

this might make sense for other SDKs where class/struct inheritance does exist, and transactions do indeed get set_operation and other methods for free by virtue of those extending the span implementation. the native SDK doesn't really programmatically support this. i also don't know if the user gains much from knowing that a transaction is loosely equivalent to a span.

sentry_value_set_by_key(span, "status", sentry_value_new_string("ok"));

if (!sentry_value_is_null(parent)) {
sentry_value_set_by_key(span, "trace_id",
sentry_value_get_by_key_owned(parent, "trace_id"));
sentry_value_set_by_key(span, "parent_span_id",
sentry_value_get_by_key_owned(parent, "span_id"));
sentry_value_set_by_key(
span, "sampled", sentry_value_get_by_key_owned(parent, "sampled"));
}

return span;
}

sentry_value_t
sentry_value_new_transaction_context(const char *name, const char *operation)
{
sentry_value_t transaction_context = sentry_value_new_object();
sentry_value_t transaction_context
= sentry__value_new_span(sentry_value_new_null(), operation);
relaxolotl marked this conversation as resolved.
Show resolved Hide resolved
sentry_transaction_context_set_name(transaction_context, name);

sentry_uuid_t trace_id = sentry_uuid_new_v4();
sentry_value_set_by_key(transaction_context, "trace_id",
Expand All @@ -1138,7 +1160,6 @@ sentry_value_new_transaction_context(const char *name, const char *operation)
transaction_context, "span_id", sentry__value_new_span_uuid(&span_id));

sentry_transaction_context_set_name(transaction_context, name);
sentry_transaction_context_set_operation(transaction_context, operation);

return transaction_context;
}
Expand All @@ -1147,14 +1168,8 @@ void
sentry_transaction_context_set_name(
sentry_value_t transaction_context, const char *name)
{
sentry_value_t sv_name = sentry_value_new_string(name);
// TODO: Consider doing this checking right before sending or flushing
// the transaction.
if (sentry_value_is_null(sv_name) || sentry__string_eq(name, "")) {
sentry_value_decref(sv_name);
sv_name = sentry_value_new_string("<unlabeled transaction>");
}
sentry_value_set_by_key(transaction_context, "name", sv_name);
sentry_value_set_by_key(
transaction_context, "transaction", sentry_value_new_string(name));
}

void
Expand Down
7 changes: 7 additions & 0 deletions src/sentry_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ sentry_value_t sentry__value_new_list_with_size(size_t size);
*/
sentry_value_t sentry__value_new_object_with_size(size_t size);

/**
* Constructs a new Span.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*
*
* Borrows `parent`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as noted in #631 (comment), i think it would be fine to not mention places where parameters are borrowed, as that's the assumed default.

*/
sentry_value_t sentry__value_new_span(
sentry_value_t parent, const char *operation);

/**
* This will parse the Value into a UUID, or return a `nil` UUID on error.
* See also `sentry_uuid_from_string`.
Expand Down
137 changes: 86 additions & 51 deletions tests/unit/test_tracing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
#include "sentry_tracing.h"
#include "sentry_uuid.h"

#define IS_NULL(Src, Field) \
Copy link
Member

Choose a reason for hiding this comment

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

maybe these helpers make sense in sentry_testsupport.h?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is too generic name. I'd rather have a sentry_key_is_null() helper function which would be a lot more readable to me. (have i mentioned i'm not a fan of macros?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seeing as there's already another helper macro in the tests right below, can i defer any relocations or changes to the macros to #637?

sentry_value_is_null(sentry_value_get_by_key(Src, Field))
#define CHECK_STRING_PROPERTY(Src, Field, Expected) \
TEST_CHECK_STRING_EQUAL( \
sentry_value_as_string(sentry_value_get_by_key(Src, Field)), Expected)

SENTRY_TEST(basic_tracing_context)
{
sentry_value_t span = sentry_value_new_object();
Expand Down Expand Up @@ -38,46 +44,31 @@ SENTRY_TEST(basic_transaction)
{
sentry_value_t tx_cxt = sentry_value_new_transaction_context(NULL, NULL);
TEST_CHECK(!sentry_value_is_null(tx_cxt));
const char *tx_name
= sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name"));
TEST_CHECK_STRING_EQUAL(tx_name, "<unlabeled transaction>");
const char *tx_op
= sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "op"));
TEST_CHECK_STRING_EQUAL(tx_op, "");
TEST_CHECK(
!sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "trace_id")));
TEST_CHECK(
!sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "span_id")));
CHECK_STRING_PROPERTY(tx_cxt, "transaction", "");
CHECK_STRING_PROPERTY(tx_cxt, "op", "");
TEST_CHECK(!IS_NULL(tx_cxt, "trace_id"));
TEST_CHECK(!IS_NULL(tx_cxt, "span_id"));

sentry_value_decref(tx_cxt);
tx_cxt = sentry_value_new_transaction_context("", "");
TEST_CHECK(!sentry_value_is_null(tx_cxt));
tx_name = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name"));
TEST_CHECK_STRING_EQUAL(tx_name, "<unlabeled transaction>");
TEST_CHECK_STRING_EQUAL(tx_op, "");
TEST_CHECK(
!sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "trace_id")));
TEST_CHECK(
!sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "span_id")));
CHECK_STRING_PROPERTY(tx_cxt, "transaction", "");
CHECK_STRING_PROPERTY(tx_cxt, "op", "");
TEST_CHECK(!IS_NULL(tx_cxt, "trace_id"));
TEST_CHECK(!IS_NULL(tx_cxt, "span_id"));

sentry_value_decref(tx_cxt);
tx_cxt = sentry_value_new_transaction_context("honk.beep", "beepbeep");
tx_name = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name"));
TEST_CHECK_STRING_EQUAL(tx_name, "honk.beep");
tx_op = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "op"));
TEST_CHECK_STRING_EQUAL(tx_op, "beepbeep");
TEST_CHECK(
!sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "trace_id")));
TEST_CHECK(
!sentry_value_is_null(sentry_value_get_by_key(tx_cxt, "span_id")));
CHECK_STRING_PROPERTY(tx_cxt, "transaction", "honk.beep");
CHECK_STRING_PROPERTY(tx_cxt, "op", "beepbeep");
TEST_CHECK(!IS_NULL(tx_cxt, "trace_id"));
TEST_CHECK(!IS_NULL(tx_cxt, "span_id"));

sentry_transaction_context_set_name(tx_cxt, "");
tx_name = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "name"));
TEST_CHECK_STRING_EQUAL(tx_name, "<unlabeled transaction>");
CHECK_STRING_PROPERTY(tx_cxt, "transaction", "");

sentry_transaction_context_set_operation(tx_cxt, "");
tx_op = sentry_value_as_string(sentry_value_get_by_key(tx_cxt, "op"));
TEST_CHECK_STRING_EQUAL(tx_op, "");
CHECK_STRING_PROPERTY(tx_cxt, "op", "");

sentry_transaction_context_set_sampled(tx_cxt, 1);
TEST_CHECK(
Expand All @@ -86,24 +77,65 @@ SENTRY_TEST(basic_transaction)
sentry_value_decref(tx_cxt);
}

static void
check_backfilled_name(sentry_envelope_t *envelope, void *data)
{
uint64_t *called = data;
*called += 1;

sentry_value_t tx = sentry_envelope_get_transaction(envelope);
TEST_CHECK(!sentry_value_is_null(tx));
CHECK_STRING_PROPERTY(tx, "transaction", "<unlabeled transaction>");

sentry_envelope_free(envelope);
}

SENTRY_TEST(transaction_name_backfill_on_finish)
{
uint64_t called = 0;

sentry_options_t *options = sentry_options_new();
sentry_options_set_dsn(options, "https://foo@sentry.invalid/42");

sentry_transport_t *transport = sentry_transport_new(check_backfilled_name);
sentry_transport_set_state(transport, &called);
sentry_options_set_transport(options, transport);

sentry_options_set_traces_sample_rate(options, 1.0);
sentry_init(options);

sentry_value_t tx_cxt = sentry_value_new_transaction_context(NULL, NULL);
sentry_value_t tx = sentry_transaction_start(tx_cxt);
sentry_uuid_t event_id = sentry_transaction_finish(tx);
TEST_CHECK(!sentry_uuid_is_nil(&event_id));

tx_cxt = sentry_value_new_transaction_context("", "");
tx = sentry_transaction_start(tx_cxt);
event_id = sentry_transaction_finish(tx);
TEST_CHECK(!sentry_uuid_is_nil(&event_id));

sentry_close();
TEST_CHECK_INT_EQUAL(called, 2);
}

static void
send_transaction_envelope_test_basic(sentry_envelope_t *envelope, void *data)
{
uint64_t *called = data;
*called += 1;

sentry_value_t transaction = sentry_envelope_get_transaction(envelope);
TEST_CHECK(!sentry_value_is_null(transaction));
const char *event_id = sentry_value_as_string(
sentry_value_get_by_key(transaction, "event_id"));
sentry_value_t tx = sentry_envelope_get_transaction(envelope);
TEST_CHECK(!sentry_value_is_null(tx));
const char *event_id
= sentry_value_as_string(sentry_value_get_by_key(tx, "event_id"));
TEST_CHECK_STRING_EQUAL(event_id, "4c035723-8638-4c3a-923f-2ab9d08b4018");

if (*called == 1) {
const char *type = sentry_value_as_string(
sentry_value_get_by_key(transaction, "type"));
const char *type
= sentry_value_as_string(sentry_value_get_by_key(tx, "type"));
TEST_CHECK_STRING_EQUAL(type, "transaction");
const char *name = sentry_value_as_string(
sentry_value_get_by_key(transaction, "transaction"));
sentry_value_get_by_key(tx, "transaction"));
TEST_CHECK_STRING_EQUAL(name, "honk");
}

Expand All @@ -126,25 +158,25 @@ SENTRY_TEST(basic_function_transport_transaction)
sentry_options_set_require_user_consent(options, true);
sentry_init(options);

sentry_value_t transaction = sentry_value_new_transaction_context(
sentry_value_t tx_cxt = sentry_value_new_transaction_context(
"How could you", "Don't capture this.");
transaction = sentry_transaction_start(transaction);
sentry_uuid_t event_id = sentry_transaction_finish(transaction);
sentry_value_t tx = sentry_transaction_start(tx_cxt);
sentry_uuid_t event_id = sentry_transaction_finish(tx);
// TODO: `sentry_capture_event` acts as if the event was sent if user
// consent was not given
TEST_CHECK(!sentry_uuid_is_nil(&event_id));
sentry_user_consent_give();

transaction = sentry_value_new_transaction_context("honk", "beep");
transaction = sentry_transaction_start(transaction);
event_id = sentry_transaction_finish(transaction);
tx_cxt = sentry_value_new_transaction_context("honk", "beep");
tx = sentry_transaction_start(tx_cxt);
event_id = sentry_transaction_finish(tx);
TEST_CHECK(!sentry_uuid_is_nil(&event_id));

sentry_user_consent_revoke();
transaction = sentry_value_new_transaction_context(
tx_cxt = sentry_value_new_transaction_context(
"How could you again", "Don't capture this either.");
transaction = sentry_transaction_start(transaction);
event_id = sentry_transaction_finish(transaction);
tx = sentry_transaction_start(tx_cxt);
event_id = sentry_transaction_finish(tx);
// TODO: `sentry_capture_event` acts as if the event was sent if user
// consent was not given
TEST_CHECK(!sentry_uuid_is_nil(&event_id));
Expand All @@ -171,10 +203,10 @@ SENTRY_TEST(transport_sampling_transactions)

uint64_t sent_transactions = 0;
for (int i = 0; i < 100; i++) {
sentry_value_t transaction
sentry_value_t tx_cxt
= sentry_value_new_transaction_context("honk", "beep");
transaction = sentry_transaction_start(transaction);
sentry_uuid_t event_id = sentry_transaction_finish(transaction);
sentry_value_t tx = sentry_transaction_start(tx_cxt);
sentry_uuid_t event_id = sentry_transaction_finish(tx);
if (!sentry_uuid_is_nil(&event_id)) {
sent_transactions += 1;
}
Expand Down Expand Up @@ -214,14 +246,17 @@ SENTRY_TEST(transactions_skip_before_send)
sentry_options_set_before_send(options, before_send, &called_beforesend);
sentry_init(options);

sentry_value_t transaction
sentry_value_t tx_cxt
= sentry_value_new_transaction_context("honk", "beep");
transaction = sentry_transaction_start(transaction);
sentry_uuid_t event_id = sentry_transaction_finish(transaction);
sentry_value_t tx = sentry_transaction_start(tx_cxt);
sentry_uuid_t event_id = sentry_transaction_finish(tx);
TEST_CHECK(!sentry_uuid_is_nil(&event_id));

sentry_close();

TEST_CHECK_INT_EQUAL(called_transport, 1);
TEST_CHECK_INT_EQUAL(called_beforesend, 0);
}

#undef IS_NULL
#undef CHECK_STRING_PROPERTY
1 change: 1 addition & 0 deletions tests/unit/tests.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ XX(session_basics)
XX(slice)
XX(symbolizer)
XX(task_queue)
XX(transaction_name_backfill_on_finish)
XX(transactions_skip_before_send)
XX(transport_sampling_transactions)
XX(uninitialized)
Expand Down