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): Add in basic Envelope support for Transactions #630

Merged
merged 1 commit into from
Dec 22, 2021
Merged

feat(tracing): Add in basic Envelope support for Transactions #630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions include/sentry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,13 +554,21 @@ typedef struct sentry_envelope_s sentry_envelope_t;
SENTRY_API void sentry_envelope_free(sentry_envelope_t *envelope);

/**
* Given an envelope returns the embedded event if there is one.
* Given an Envelope, returns the embedded Event if there is one.
*
* This returns a borrowed value to the event in the envelope.
* This returns a borrowed value to the Event in the Envelope.
*/
SENTRY_API sentry_value_t sentry_envelope_get_event(
const sentry_envelope_t *envelope);

/**
* Given an Envelope, returns the embedded Transaction if there is one.
*
* This returns a borrowed value to the Transaction in the Envelope.
*/
SENTRY_EXPERIMENTAL_API sentry_value_t sentry_envelope_get_transaction(
const sentry_envelope_t *envelope);

/**
* Serializes the envelope.
*
Expand Down
62 changes: 61 additions & 1 deletion src/sentry_envelope.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ envelope_add_item(sentry_envelope_t *envelope)
if (envelope->contents.items.item_count >= SENTRY_MAX_ENVELOPE_ITEMS) {
return NULL;
}
// TODO: Envelopes may have at most one event item or one transaction item,
// and not one of both. Some checking should be done here or in
// `sentry__envelope_add_[transaction|event]` to ensure this can't happen.

sentry_envelope_item_t *rv
= &envelope->contents.items
Expand Down Expand Up @@ -197,7 +200,25 @@ sentry_envelope_get_event(const sentry_envelope_t *envelope)
return sentry_value_new_null();
}
for (size_t i = 0; i < envelope->contents.items.item_count; i++) {
if (!sentry_value_is_null(envelope->contents.items.items[i].event)) {
if (!sentry_value_is_null(envelope->contents.items.items[i].event)
&& !sentry__event_is_transaction(
envelope->contents.items.items[i].event)) {
return envelope->contents.items.items[i].event;
}
}
return sentry_value_new_null();
}

sentry_value_t
sentry_envelope_get_transaction(const sentry_envelope_t *envelope)
{
if (envelope->is_raw) {
return sentry_value_new_null();
}
for (size_t i = 0; i < envelope->contents.items.item_count; i++) {
if (!sentry_value_is_null(envelope->contents.items.items[i].event)
&& sentry__event_is_transaction(
envelope->contents.items.items[i].event)) {
return envelope->contents.items.items[i].event;
}
}
Expand Down Expand Up @@ -234,6 +255,45 @@ sentry__envelope_add_event(sentry_envelope_t *envelope, sentry_value_t event)
return item;
}

sentry_envelope_item_t *
sentry__envelope_add_transaction(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a majority of this is just copypasted from sentry__envelope_add_event, with a few differences:

  • this sets a different item header value
  • this sets a sent_at header value

this is duplicated to minimize impact on the event implementation, and to mostly follow the pattern established by other SDKs (they also have separate functions to add different types of events to an envelope).

of course, this misses out on some of the nice perks that the other SDKs gain from having this pattern, but i think minimizing the impact of this change is enough to justify this for now. open to changing my opinion on this, though. if merged into sentry__envelope_add_event, this would only introduce 1 or 2 additional if statements (for now).

Copy link
Member

Choose a reason for hiding this comment

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

I think the sent_at value is applicable to all envelopes? Should we rather add this really at "send" time, as opposed to "adding item to envelope" time?

Also, maybe split this into a sentry__envelope_add_value(sentry_value_t value, const char *type) that rather takes the type header value explicitly?

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 the sent_at value is applicable to all envelopes? Should we rather add this really at "send" time, as opposed to "adding item to envelope" time?

i wouldn't be opposed to this, although it looks like all of the code paths that use this (and sentry__envelope_add_event) send the envelope immediately afterwards. this means that any inaccuracies or timing offsets insent_at would be caused by things like adding/ending the current session, or the amount of time it takes to push it completely through the transport.

what do you think of the idea of documenting this somewhere, and if somebody notices any issues with the sent_at timestamps we can set the header a little bit closer to when it's actually sent? something to consider is that events being sent by the native SDK right now (not transactions) do not have the sent_at header. i'm not sure if it's because the header precedes this implementation, or if there's some sort of subtle timing issue that lead to this omission.

maybe split this into a sentry__envelope_add_value(sentry_value_t value, const char *type) that rather takes the type header value explicitly?

this is a good idea and i'll give this a try in a follow-up PR, if that's alright with you. it should be a simple change, hopefully.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, go for it in a followup. And the suggestion for sent_at seems to be moot as well. I don’t think we need that much precision after all :-D

sentry_envelope_t *envelope, sentry_value_t transaction)
{
sentry_envelope_item_t *item = envelope_add_item(envelope);
if (!item) {
return NULL;
}

sentry_jsonwriter_t *jw = sentry__jsonwriter_new(NULL);
if (!jw) {
return NULL;
}

sentry_value_t event_id = sentry__ensure_event_id(transaction, NULL);

item->event = transaction;
sentry__jsonwriter_write_value(jw, transaction);
Comment on lines +274 to +275
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't really wrong because the value is still alive. but rust wouldn't let you do this since sentry__jsonwriter_write_value borrows a value which you just passed ownership away from on the previous line.

item->payload = sentry__jsonwriter_into_string(jw, &item->payload_len);

sentry__envelope_item_set_header(
item, "type", sentry_value_new_string("transaction"));
sentry_value_t length = sentry_value_new_int32((int32_t)item->payload_len);
sentry__envelope_item_set_header(item, "length", length);

sentry_value_incref(event_id);
sentry__envelope_set_header(envelope, "event_id", event_id);

#ifdef SENTRY_UNITTEST
sentry_value_t now = sentry_value_new_string("2021-12-16T05:53:59.343Z");
#else
sentry_value_t now = sentry__value_new_string_owned(
sentry__msec_time_to_iso8601(sentry__msec_time()));
#endif
sentry__envelope_set_header(envelope, "sent_at", now);

return item;
}

sentry_envelope_item_t *
sentry__envelope_add_session(
sentry_envelope_t *envelope, const sentry_session_t *session)
Expand Down
6 changes: 6 additions & 0 deletions src/sentry_envelope.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ sentry_uuid_t sentry__envelope_get_event_id(const sentry_envelope_t *envelope);
sentry_envelope_item_t *sentry__envelope_add_event(
sentry_envelope_t *envelope, sentry_value_t event);

/**
* Add a transaction to this envelope.
*/
sentry_envelope_item_t *sentry__envelope_add_transaction(
sentry_envelope_t *envelope, sentry_value_t transaction);

/**
* Add a session to this envelope.
*/
Expand Down
32 changes: 32 additions & 0 deletions tests/unit/test_envelopes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,38 @@ SENTRY_TEST(basic_http_request_preparation_for_event)
sentry__dsn_decref(dsn);
}

SENTRY_TEST(basic_http_request_preparation_for_transaction)
{
sentry_dsn_t *dsn = sentry__dsn_new("https://[email protected]/42");

sentry_uuid_t event_id
= sentry_uuid_from_string("c993afb6-b4ac-48a6-b61b-2558e601d65d");
sentry_envelope_t *envelope = sentry__envelope_new();
sentry_value_t transaction = sentry_value_new_object();
sentry_value_set_by_key(
transaction, "event_id", sentry__value_new_uuid(&event_id));
sentry_value_set_by_key(
transaction, "type", sentry_value_new_string("transaction"));
sentry__envelope_add_transaction(envelope, transaction);

sentry_prepared_http_request_t *req
= sentry__prepare_http_request(envelope, dsn, NULL);
TEST_CHECK_STRING_EQUAL(req->method, "POST");
TEST_CHECK_STRING_EQUAL(
req->url, "https://sentry.invalid:443/api/42/envelope/");
TEST_CHECK_STRING_EQUAL(req->body,
"{\"event_id\":\"c993afb6-b4ac-48a6-b61b-2558e601d65d\",\"sent_at\":"
"\"2021-12-16T05:53:59.343Z\"}\n"
"{\"type\":\"transaction\",\"length\":72}\n"
"{\"event_id\":\"c993afb6-b4ac-48a6-b61b-2558e601d65d\",\"type\":"
"\"transaction\"}");

sentry__prepared_http_request_free(req);
sentry_envelope_free(envelope);

sentry__dsn_decref(dsn);
}

SENTRY_TEST(basic_http_request_preparation_for_event_with_attachment)
{
sentry_dsn_t *dsn = sentry__dsn_new("https://[email protected]/42");
Expand Down
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 @@ -4,6 +4,7 @@ XX(basic_function_transport)
XX(basic_http_request_preparation_for_event)
XX(basic_http_request_preparation_for_event_with_attachment)
XX(basic_http_request_preparation_for_minidump)
XX(basic_http_request_preparation_for_transaction)
XX(basic_tracing_context)
XX(basic_transaction)
XX(buildid_fallback)
Expand Down