-
-
Notifications
You must be signed in to change notification settings - Fork 175
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): Basic transaction context creation #619
Changes from 6 commits
ccb367d
190079d
6681a6c
02c58f5
f47920c
d4c2df3
25cd4d5
767e7e0
a7b649a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1124,6 +1124,53 @@ sentry_value_new_stacktrace(void **ips, size_t len) | |
return stacktrace; | ||
} | ||
|
||
sentry_value_t | ||
sentry_value_new_transaction_context(const char *name, const char *operation) | ||
{ | ||
sentry_value_t transaction_context = sentry_value_new_object(); | ||
|
||
sentry_transaction_context_set_name(transaction_context, name); | ||
sentry_transaction_context_set_operation(transaction_context, operation); | ||
|
||
return transaction_context; | ||
} | ||
|
||
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>"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we discussed elsewhere, not sure if we should do validation here. But I don’t object to it either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'll leave a note for the time being so the validation is still being done somewhere (in case it gets forgotten). the only other place where this validation makes sense to occur is yet to show up in a proper PR, so i think we can revisit this conversation once the implementation for that appears. |
||
} | ||
sentry_value_set_by_key(transaction_context, "name", sv_name); | ||
} | ||
|
||
void | ||
sentry_transaction_context_set_operation( | ||
sentry_value_t transaction_context, const char *operation) | ||
{ | ||
sentry_value_set_by_key( | ||
transaction_context, "op", sentry_value_new_string(operation)); | ||
} | ||
|
||
void | ||
sentry_transaction_context_set_sampled( | ||
sentry_value_t transaction_context, int sampled) | ||
{ | ||
sentry_value_set_by_key( | ||
transaction_context, "sampled", sentry_value_new_bool(sampled)); | ||
} | ||
|
||
void | ||
sentry_transaction_context_remove_sampled(sentry_value_t transaction_context) | ||
{ | ||
sentry_value_remove_by_key(transaction_context, "sampled"); | ||
} | ||
|
||
static sentry_value_t | ||
sentry__get_or_insert_values_list(sentry_value_t parent, const char *key) | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm strongly considering renaming all of these and stripping the
_context
bit of this. instead, the API would most likely look more like this:sentry_value_new_transaction
sentry_transaction_set_name
sentry_transaction_set_operation
sentry_transaction_set_sampled
sentry_start_transaction
sentry_transaction_finish
it does mean that we paper over the difference between an inert transaction and one that is active, but you could argue that most objects are the same way (see Exceptions, Threads, Messages, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still relevant despite the outdated tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be ok with your newly proposed names as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sentry_start_transaction
kind of sticks out among those name tbh. Should that be renamed tosentry_transaction_start
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me get some opinions from some of the people working on perfmon to see if they're averse to us changing the name a little in native - if they're adamant about having a function named
start_transaction
, perhaps we can just offer both options (sentry_start_transaction
andsentry_transaction_start
) and let users decide?