feat(tracing): Basic transaction context creation #619

relaxolotl · 2021-12-10T04:20:36Z

Nothing new for those who have been keeping up with #618.

This adds in the ability to create transaction contexts as defined in https://develop.sentry.dev/sdk/performance/#new-span-and-transaction-classes, under Transaction Interface. Fields are based on the cocoa SDK's implementation. Defaults for values are based on the python SDK and js SDK implementations.

Instead of defining several transaction constructor functions with varying names (since overloading doesn't exist), the decision has been made to have the user construct an "inactive" transaction which should be fed into the SDK's implementation of start_transaction. This follows an existing pattern in the SDK where exceptions, threads, messages, etc can be constructed but they must be explicitly added to an event to be sent to sentry.

The implementation is missing a few key things:

constructors from a raw sentry-trace header as well as a header map (ie decoded and parsed sentry-trace header).

relates to #601

relaxolotl · 2021-12-10T04:37:48Z

i'm a little baffled by the build failure; probably need to pick at somebody else's brain to see what's going wrong here.

Swatinem · 2021-12-10T10:13:05Z

src/sentry_value.c

+    sentry_value_t sv_name = sentry_value_new_string(name);
+    if (sentry__string_eq(name, "") || sentry_value_is_null(sv_name)) {
+        sentry_value_decref(sv_name);
+        sv_name = sentry_value_new_string("<unlabeled transaction>");


As we discussed elsewhere, not sure if we should do validation here. But I don’t object to it either.

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.

tests/unit/test_tracing.c

codecov-commenter · 2021-12-11T01:28:56Z

Codecov Report

Merging #619 (4ac8675) into tracing/set-trace-context (1751cdb) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@                      Coverage Diff                      @@
##           tracing/set-trace-context     #619      +/-   ##
=============================================================
+ Coverage                      86.68%   86.77%   +0.08%     
=============================================================
  Files                             50       50              
  Lines                           4012     4024      +12     
  Branches                         858      859       +1     
=============================================================
+ Hits                            3478     3492      +14     
+ Misses                           446      445       -1     
+ Partials                          88       87       -1

relaxolotl · 2021-12-14T02:15:07Z

not sure what changed over the weekend but clearly something broke on macOS. i'll have to investigate what's happening here because it's probably (?) not a functionality change if only one platform is consistently failing.

the integration tests aren't failing on my local machine either, so i can't even reproduce this locally. i wonder if this could be related to M1 work?

relaxolotl · 2021-12-14T04:42:21Z

include/sentry.h

+SENTRY_EXPERIMENTAL_API sentry_value_t sentry_value_new_transaction_context(
+    const char *name, const char *operation);
+
+/**
+ * Sets the name of a transaction on a `transaction_context`.
+ */
+SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_name(
+    sentry_value_t transaction_context, const char *name);
+
+/**
+ * Sets the operation of a transaction on a `transaction_context`.
+ *
+ * See https://develop.sentry.dev/sdk/performance/span-operations/ for
+ * conventions on `operation`s.
+ */
+SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_operation(
+    sentry_value_t transaction_context, const char *operation);
+
+/**
+ * Sets the sampled field on a `transaction_context`. When turned on, the
+ * transaction constructed from the `transaction_context` will bypass all
+ * sampling options and always be sent to sentry. If this is explicitly turned
+ * off in the `transaction_context`, the transaction will never be sent to
+ * sentry.
+ */
+SENTRY_EXPERIMENTAL_API void sentry_transaction_context_set_sampled(
+    sentry_value_t transaction_context, int sampled);
+
+/**
+ * Removes the sampled field on a `transaction_context`. The transaction
+ * contructed from it will use the sampling rate as defined in `sentry_options`.
+ */
+SENTRY_EXPERIMENTAL_API void sentry_transaction_context_remove_sampled(
+    sentry_value_t transaction_context);


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)

this is still relevant despite the outdated tag.

I'd be ok with your newly proposed names as well

sentry_start_transaction kind of sticks out among those name tbh. Should that be renamed to sentry_transaction_start?

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 and sentry_transaction_start) and let users decide?

relaxolotl · 2021-12-15T03:07:19Z

somehow there's a mismatch in macos versions after github switched macos-latest to use macos-11. previously the ci was passing when macos-latest still pointed to macos-10.15, but after the switch sys.platform in the integration tests still reports that it's on 10.16, causing them to fail.

relaxolotl · 2021-12-15T05:29:57Z

big sur is also 10.16 for backwards compatibility... it sounds like setting SYSTEM_VERSION_COMPAT=0 might get it to return the value we need, but i'm going to switch ci back to 10.15 if that doesn't work.

relaxolotl · 2021-12-15T05:46:53Z

looks like it works, pulling this out into its own PR.

relaxolotl · 2021-12-15T06:12:29Z

going to keep the commit here so we can stop getting falsey negatives for the diff as i make changes to the code, hoping the diff will resolve itself neatly once the PR that fixes the ci makes it into master

* ci(codechecker): Workaround for code checker not building due to node issues (getsentry#615) * meta: Update breakpad/crashpad to 2021-12-03 (getsentry#614) * feat(tracing): Add config options (getsentry#613) * fix: Correct changelog entry (getsentry#622) * meta: Bump breakpad (getsentry#621) * feat: Add internal UUID types (getsentry#616) This adds in support for internal UUIDs needed by tracing, such as the trace ID and the span ID. The major difference between this and the "standard" UUID is that the hyphens are stripped during serialization. sentry appears to not consider the hyphenated representations of these UUIDs to be valid for certain fields in an event. * meta: Update changelog (getsentry#625) * release: 0.4.13 * feat(tracing): Groundwork to add tracing context to all events (getsentry#617) This adds the appropriate stubs and fields to start storing spans on the (universal) scope. No actual logic has been added to actually support setting spans on the scope itself. The focus of this is to begin including tracing info in the context on all events if there is a transaction set on the scope. It does this fairly naively right now as the tooling to merge `sentry_value_t`s are basically nonexistent. * ci: Make integration tests capable of reading the non-backwards compatible version number for Big Sur (getsentry#627) * feat(tracing): Basic transaction context creation (getsentry#619) This adds in the ability to create and manipulate transaction contexts as defined in https://develop.sentry.dev/sdk/performance/#new-span-and-transaction-classes, under Transaction Interface. Instead of defining several transaction constructor functions with varying names (since overloading doesn't exist), the decision has been made to have the user construct an "inactive" transaction which should be fed into the SDK's implementation of `start_transaction`. This follows an existing pattern in the SDK where exceptions, threads, messages, etc can be constructed but they must be explicitly added to an event to be sent to sentry. * feat(tracing): Support basic sampling of transactions (getsentry#620) If an event is a transaction, event flushing should determine discard or forward the transaction to sentry based on the sample rate as configured in sentry options. Follows the sampling rules as defined in https://develop.sentry.dev/sdk/performance/#sampling-context. This does not take into consideration parent sampling as that property is currently unimplemented on the transaction context. * feat(tracing): Introduce a helper that identifies events that are transactions (getsentry#628) * feat(tracing): Restrict `sentry_capture_event` so it only sends non-transaction events (getsentry#629) Prevent the public API from being used to send transaction events as another transaction-specific function is meant to be used to accomplish this. * fix: Avoid deadlocks with uninitialized options (getsentry#639) The `SENTRY_WITH_OPTIONS_MUT` was a footgun since it never unlocked when the options were NULL (uninitialized). This removes the macro and replaces its uses with explicit lock/unlock calls. * feat(tracing): Add in basic Envelope support for Transactions (getsentry#630) * feat(tracing): Allow manual creation and sending of spanless Transactions (getsentry#631) * feat(tracing): Defer some transaction validation and allow creation of internal spans (getsentry#633) Co-authored-by: relaxolotl <5597345+relaxolotl@users.noreply.github.com> Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com> Co-authored-by: getsentry-bot <bot@sentry.io> Co-authored-by: Arpad Borsos <swatinem@swatinem.de> Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>

) * feat: Add support for Qt 6 (getsentry#509) * fix: Windows SDK Compiler Warning (getsentry#511) * fix: Validate tm put into strftime (getsentry#510) * fix: Rewrite the Linux module finder (getsentry#431) It now works in memory, without requiring to mmap the libraries again, which should make it work correctly on android when loading libraries directly from apk or appbundle files. The new code will keep track of readable memory maps and will translate read requests based on the offset of those memory maps, making sure that we actually can read whatever we are trying to read. * build: Avoid building all targets (getsentry#512) It looks like cmake is broken and builds ALL the targets when the parallel option is specified first, lol * fix: Update Crashpad to 2021-04-12 and fix macOS universal build (getsentry#513) * feat: Invoke before_send hook when using Crashpad (getsentry#519) * feat: Add more Event Payload convenience methods (getsentry#517) Adds: * `sentry_value_new_exception` * `sentry_value_new_thread` * `sentry_value_new_stacktrace` * `sentry_event_add_exception` * `sentry_event_add_thread` Deprecates `sentry_event_value_add_stacktrace` * feat: Introduce `sentry_close` (getsentry#518) This replaces the former `sentry_shutdown`, which is being forwarded. * meta: Prepare Changelog for upcoming release (getsentry#522) * ref: Pass options to scope apply directly (getsentry#521) * fix: Further clean up platform libraries for static linking (getsentry#523) * fix: Better macOS availability checks (getsentry#524) This should allow building on older macOS versions as well as running on older versions by fixing the usage of __builtin_available, and adding a different clock source for older macOS versions. * release: 0.4.9 * fix: Avoid double-free on invalid DSN (getsentry#527) * meta: Use correct libunwindstack commit * fix: Allow for Unity builds (getsentry#536) * ref: Add more testcases that trigger crashes in various ways (getsentry#538) * ref(craft): Modernize Craft config (getsentry#543) * fix: Update venv and test-discovery Makefile targets (getsentry#544) * fix: Avoid recursion when using `sentry_reinstall_backend` (getsentry#548) Previously, the `inproc` and `crashpad` (on linux) backends didn’t correctly reset their signal handlers when doing `reinstall_backend` (or multiple `init` calls for that matter). This has led to an infinite loop generating crashes. The fix now correctly unregisters the inproc/crashpad signal handlers, and adds an integration test using `reinstall_backend` to make sure we do not end up in an infinite loop. Co-authored-by: Mischa Alff <mischa@mutate.se> * fix: Address -Wundef warning for SENTRY_UNITTEST defines (getsentry#549) * build: Set 32-bit option for compiling assembly as well (getsentry#550) This fixes compilation of breakpad for 32-bit systems * meta: Update break/crashpad to 2021-06-14 (getsentry#552) * fix: Shorten/Split Locked sections to avoid deadlock (getsentry#551) We have received a report that the `sentry_get_modules_list` on mac can deadlock when other code concurrently does a `dlopen` and thus invokes the `add_image` callback from a different thread. We shorten/split the locked blocks in order to avoid holding a lock in the `get_modules` function whenever the `add_image` function is being invoked possibly from other threads. * fix: Tighten Stack Usage (getsentry#553) According to some docs, JVM/JNI stacks on Android can be as small as 32K, and our own sigaltstack is not much larger with 64K. Make sure to avoid large stack allocations as much as possible. We have especially seen the literal content of `/proc/self/maps` as well as formatted addresses inside corrupted release/environment attributes, which might point to overflows that write into a previously allocated release/environment string. * meta: Update Changelog (getsentry#556) * release: 0.4.10 * reformat * fix: Make Linux modulefinder/unwinder safer (getsentry#559) This is using the `process_vm_read` call to safely poke at random memory. It also makes sure to shim the libc provided call with a direct syscall for older Android devices. * docs: Try to better explain unwind API (getsentry#564) * fix: Make Crashpad Backend respect max_breadcrumbs setting (getsentry#566) * fix: Cancel slow winhttp requests on shutdown (getsentry#570) Co-authored-by: Gerhard Herbert <gerhard.herbert@avira.com> * fix: Properly close the background worker thread on timeout (getsentry#571) * fix: Possible race conditions in init/close and sessions (getsentry#545) * meta: Draft Changelog (getsentry#572) * release: 0.4.11 * feat: Make shutdown timeout customizable (getsentry#577) Co-authored-by: Andrei Muraru <andrei.muraru@avira.com> * CMake: Link to the CURL::libcurl target when available (getsentry#579) Caters better for newer cmake versions. * meta: Update crashpad to 2021-07-14 (getsentry#580) * fix: Properly use `SENTRY_BUILD_RUNTIMESTATIC` for `sentry_fuzz_json` unit test (getsentry#583) * meta: Update break/crashpad to 2021-07-28 (getsentry#584) * release: 0.4.12 * fix: Increment CXX standard to 14 to allow crashpad build (getsentry#585) Fixes getsentry#574 * meta: Bump python dependencies (getsentry#600) The old version of pytest breaks with python 3.10 which changed a little how code object internals work. Since python 3.10 is now released it starts being used in CI. * fix: Ensure that a valid DSN has a public_key (getsentry#598) * feat: AIX support (getsentry#593) * CMake: Check whether libcurl was already found (getsentry#602) Currently when there is any other project that brings libcurl as a dependency, the build fails with “Could NOT find CURL (missing: CURL_LIBRARY CURL_INCLUDE_DIR)“, even though libcurl has already added as CURL::libcurl library. This patch adds a check for CURL_FOUND, to indicate that the library was already found, if set by another project. It also skips the additional find_package() step so it does not fail. Signed-off-by: Ladislav Macoun <ladislavmacoun@gmail.com> * CMake: fix `SENTRY_BACKEND` defined in outer scope (getsentry#603) * CMake: add ability to set solution folder name (getsentry#604) * [pull] master from getsentry:master (#14) * ci(codechecker): Workaround for code checker not building due to node issues (getsentry#615) * meta: Update breakpad/crashpad to 2021-12-03 (getsentry#614) * feat(tracing): Add config options (getsentry#613) * fix: Correct changelog entry (getsentry#622) * meta: Bump breakpad (getsentry#621) * feat: Add internal UUID types (getsentry#616) This adds in support for internal UUIDs needed by tracing, such as the trace ID and the span ID. The major difference between this and the "standard" UUID is that the hyphens are stripped during serialization. sentry appears to not consider the hyphenated representations of these UUIDs to be valid for certain fields in an event. * meta: Update changelog (getsentry#625) * release: 0.4.13 * feat(tracing): Groundwork to add tracing context to all events (getsentry#617) This adds the appropriate stubs and fields to start storing spans on the (universal) scope. No actual logic has been added to actually support setting spans on the scope itself. The focus of this is to begin including tracing info in the context on all events if there is a transaction set on the scope. It does this fairly naively right now as the tooling to merge `sentry_value_t`s are basically nonexistent. * ci: Make integration tests capable of reading the non-backwards compatible version number for Big Sur (getsentry#627) * feat(tracing): Basic transaction context creation (getsentry#619) This adds in the ability to create and manipulate transaction contexts as defined in https://develop.sentry.dev/sdk/performance/#new-span-and-transaction-classes, under Transaction Interface. Instead of defining several transaction constructor functions with varying names (since overloading doesn't exist), the decision has been made to have the user construct an "inactive" transaction which should be fed into the SDK's implementation of `start_transaction`. This follows an existing pattern in the SDK where exceptions, threads, messages, etc can be constructed but they must be explicitly added to an event to be sent to sentry. * feat(tracing): Support basic sampling of transactions (getsentry#620) If an event is a transaction, event flushing should determine discard or forward the transaction to sentry based on the sample rate as configured in sentry options. Follows the sampling rules as defined in https://develop.sentry.dev/sdk/performance/#sampling-context. This does not take into consideration parent sampling as that property is currently unimplemented on the transaction context. * feat(tracing): Introduce a helper that identifies events that are transactions (getsentry#628) * feat(tracing): Restrict `sentry_capture_event` so it only sends non-transaction events (getsentry#629) Prevent the public API from being used to send transaction events as another transaction-specific function is meant to be used to accomplish this. * fix: Avoid deadlocks with uninitialized options (getsentry#639) The `SENTRY_WITH_OPTIONS_MUT` was a footgun since it never unlocked when the options were NULL (uninitialized). This removes the macro and replaces its uses with explicit lock/unlock calls. * feat(tracing): Add in basic Envelope support for Transactions (getsentry#630) * feat(tracing): Allow manual creation and sending of spanless Transactions (getsentry#631) * feat(tracing): Defer some transaction validation and allow creation of internal spans (getsentry#633) Co-authored-by: relaxolotl <5597345+relaxolotl@users.noreply.github.com> Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com> Co-authored-by: getsentry-bot <bot@sentry.io> Co-authored-by: Arpad Borsos <swatinem@swatinem.de> Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io> Co-authored-by: Tor Arne Vestbø <torarnv@gmail.com> Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io> Co-authored-by: Luke Street <luke@street.dev> Co-authored-by: getsentry-bot <bot@getsentry.com> Co-authored-by: Sentry Bot <getsentry-bot@users.noreply.github.com> Co-authored-by: Arpad Borsos <swatinem@swatinem.de> Co-authored-by: bschatt <44769431+bschatt@users.noreply.github.com> Co-authored-by: Burak Yigit Kaya <byk@sentry.io> Co-authored-by: MikeRumplerSentry <85497711+MikeRumplerSentry@users.noreply.github.com> Co-authored-by: Mischa Alff <mischa@mutate.se> Co-authored-by: Michał Janiszewski <janisozaur@users.noreply.github.com> Co-authored-by: getsentry-bot <bot@sentry.io> Co-authored-by: Gerhard Herbert <gerhard.herbert@avira.com> Co-authored-by: andrei-mu <andrei5008@gmail.com> Co-authored-by: Andrei Muraru <andrei.muraru@avira.com> Co-authored-by: pastdue <30942300+past-due@users.noreply.github.com> Co-authored-by: Roshan Padaki <roshanpadaki@gmail.com> Co-authored-by: mjvankampen <markjanvankampen@gmail.com> Co-authored-by: Floris Bruynooghe <flub@sentry.io> Co-authored-by: Calvin Buckley <calvin@cmpct.info> Co-authored-by: Ladislav <ladislavmacoun@gmail.com> Co-authored-by: Mikhail Paulyshka <me@mixaill.tk> Co-authored-by: pull[bot] <39814207+pull[bot]@users.noreply.github.com> Co-authored-by: relaxolotl <5597345+relaxolotl@users.noreply.github.com> Co-authored-by: Sebastian Zivota <loewenheim@users.noreply.github.com>

relaxolotl mentioned this pull request Dec 10, 2021

feat(tracing): Support basic sampling of transactions #620

Merged

Swatinem approved these changes Dec 10, 2021

View reviewed changes

Swatinem reviewed Dec 10, 2021

View reviewed changes

tests/unit/test_tracing.c Outdated Show resolved Hide resolved

relaxolotl force-pushed the tracing/set-trace-context branch from 148e3e1 to 1751cdb Compare December 10, 2021 20:18

relaxolotl force-pushed the tracing/transaction-context branch from 7e9f636 to 3a6a9f8 Compare December 10, 2021 20:19

loewenheim approved these changes Dec 13, 2021

View reviewed changes

Base automatically changed from tracing/set-trace-context to master December 13, 2021 23:56

relaxolotl added 3 commits December 13, 2021 19:06

feat(tracing): Basic transaction context creation

ccb367d

remove redundant import that's probably breaking builds

190079d

leave a note

6681a6c

relaxolotl force-pushed the tracing/transaction-context branch from a031aae to 6681a6c Compare December 14, 2021 00:06

relaxolotl marked this pull request as ready for review December 14, 2021 00:12

relaxolotl mentioned this pull request Dec 14, 2021

barebones transaction work #618

Closed

allow removal of the sampled field

02c58f5

relaxolotl added 2 commits December 13, 2021 22:51

strongly hint at setting an operation

f47920c

some explanation about properties

d4c2df3

relaxolotl commented Dec 14, 2021

View reviewed changes

more doc consistency

25cd4d5

try fixing ci

767e7e0

s/transaction_context/transaction

a7b649a

relaxolotl merged commit f98a9d7 into master Dec 15, 2021

relaxolotl deleted the tracing/transaction-context branch December 15, 2021 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracing): Basic transaction context creation #619

feat(tracing): Basic transaction context creation #619

relaxolotl commented Dec 10, 2021 •

edited

Loading

relaxolotl commented Dec 10, 2021

Swatinem Dec 10, 2021

relaxolotl Dec 11, 2021

codecov-commenter commented Dec 11, 2021 •

edited

Loading

relaxolotl commented Dec 14, 2021 •

edited

Loading

relaxolotl Dec 14, 2021

relaxolotl Dec 14, 2021

flub Dec 14, 2021

loewenheim Dec 15, 2021

relaxolotl Dec 15, 2021

relaxolotl commented Dec 15, 2021

relaxolotl commented Dec 15, 2021 •

edited

Loading

relaxolotl commented Dec 15, 2021

relaxolotl commented Dec 15, 2021

feat(tracing): Basic transaction context creation #619

feat(tracing): Basic transaction context creation #619

Conversation

relaxolotl commented Dec 10, 2021 • edited Loading

relaxolotl commented Dec 10, 2021

Swatinem Dec 10, 2021

Choose a reason for hiding this comment

relaxolotl Dec 11, 2021

Choose a reason for hiding this comment

codecov-commenter commented Dec 11, 2021 • edited Loading

Codecov Report

relaxolotl commented Dec 14, 2021 • edited Loading

relaxolotl Dec 14, 2021

Choose a reason for hiding this comment

relaxolotl Dec 14, 2021

Choose a reason for hiding this comment

flub Dec 14, 2021

Choose a reason for hiding this comment

loewenheim Dec 15, 2021

Choose a reason for hiding this comment

relaxolotl Dec 15, 2021

Choose a reason for hiding this comment

relaxolotl commented Dec 15, 2021

relaxolotl commented Dec 15, 2021 • edited Loading

relaxolotl commented Dec 15, 2021

relaxolotl commented Dec 15, 2021

relaxolotl commented Dec 10, 2021 •

edited

Loading

codecov-commenter commented Dec 11, 2021 •

edited

Loading

relaxolotl commented Dec 14, 2021 •

edited

Loading

relaxolotl commented Dec 15, 2021 •

edited

Loading