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

relaxolotl · 2021-12-18T04:36:29Z

This moves some validation on transaction fields from the setter to sentry_transaction_finish. As the validity of these fields aren't crucial to most of the transaction's lifecycle, I figured it was been safe to move it to the end of that very lifecycle. A minor rename was made to the same field.

Since this code was touching the same places already, this also introduces a span creation function which the transaction creator now relies on. This follows the general recommendations made by https://develop.sentry.dev/sdk/performance/ regarding how spans and transactions should be structured and their relationship to one another (transactions should be specialized spans).

relates to #601

picks up off of #631

Swatinem · 2021-12-21T10:55:10Z

tests/unit/test_tracing.c

@@ -3,6 +3,8 @@
 #include "sentry_tracing.h"
 #include "sentry_uuid.h"

+#define IS_NULL(Src, Field)                                                    \


maybe these helpers make sense in sentry_testsupport.h?

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?)

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?

flub · 2021-12-21T14:58:02Z

src/sentry_core.c

+    // 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


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.

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.

flub · 2021-12-21T15:09:47Z

src/sentry_value.c

+{
+    sentry_value_t span = sentry_value_new_object();
+
+    sentry_transaction_context_set_operation(span, operation);


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.

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:

there's no such thing as "inheritance" in this to begin with

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.

flub · 2021-12-21T15:17:28Z

src/sentry_value.h

@@ -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.
+ *


Suggested change

*

*

* Borrows `parent`.

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.

src/sentry_value.c

flub · 2021-12-21T15:27:08Z

tests/unit/test_tracing.c

@@ -3,6 +3,8 @@
 #include "sentry_tracing.h"
 #include "sentry_uuid.h"

+#define IS_NULL(Src, Field)                                                    \


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?)

relaxolotl · 2021-12-23T01:35:44Z

fyi: this has been detached from #632's changes and attached to #631 instead, meaning that this can be merged once the latter lands on master. this is because #632's changes are likely to be contentious or will most likely go through a bit of churn because of the nature of its changes, and this PR doesn't particularly rely on #632's changes. it mostly relies on #631.

…f internal spans

* 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 <[email protected]> Co-authored-by: Sebastian Zivota <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: Arpad Borsos <[email protected]> Co-authored-by: Arpad Borsos <[email protected]>

) * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Sebastian Zivota <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: Arpad Borsos <[email protected]> Co-authored-by: Arpad Borsos <[email protected]> Co-authored-by: Tor Arne Vestbø <[email protected]> Co-authored-by: Arpad Borsos <[email protected]> Co-authored-by: Luke Street <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: Sentry Bot <[email protected]> Co-authored-by: Arpad Borsos <[email protected]> Co-authored-by: bschatt <[email protected]> Co-authored-by: Burak Yigit Kaya <[email protected]> Co-authored-by: MikeRumplerSentry <[email protected]> Co-authored-by: Mischa Alff <[email protected]> Co-authored-by: Michał Janiszewski <[email protected]> Co-authored-by: getsentry-bot <[email protected]> Co-authored-by: Gerhard Herbert <[email protected]> Co-authored-by: andrei-mu <[email protected]> Co-authored-by: Andrei Muraru <[email protected]> Co-authored-by: pastdue <[email protected]> Co-authored-by: Roshan Padaki <[email protected]> Co-authored-by: mjvankampen <[email protected]> Co-authored-by: Floris Bruynooghe <[email protected]> Co-authored-by: Calvin Buckley <[email protected]> Co-authored-by: Ladislav <[email protected]> Co-authored-by: Mikhail Paulyshka <[email protected]> Co-authored-by: pull[bot] <39814207+pull[bot]@users.noreply.github.com> Co-authored-by: relaxolotl <[email protected]> Co-authored-by: Sebastian Zivota <[email protected]>

relaxolotl requested a review from a team December 18, 2021 04:36

relaxolotl force-pushed the tracing/lazy-validation branch 2 times, most recently from 7802f17 to ef766be Compare December 18, 2021 04:59

relaxolotl mentioned this pull request Dec 18, 2021

feat(tracing): Basic span support with nesting #634

Merged

loewenheim approved these changes Dec 20, 2021

View reviewed changes

relaxolotl force-pushed the tracing/automanaged-tx branch from c339d4f to ceddb01 Compare December 21, 2021 08:11

relaxolotl force-pushed the tracing/lazy-validation branch from ef766be to cbf156a Compare December 21, 2021 08:25

Swatinem approved these changes Dec 21, 2021

View reviewed changes

flub reviewed Dec 21, 2021

View reviewed changes

relaxolotl mentioned this pull request Dec 23, 2021

feat(tracing): Allow manual creation and sending of spanless Transactions #631

Merged

relaxolotl force-pushed the tracing/lazy-validation branch from cbf156a to 6e57ae4 Compare December 23, 2021 01:33

relaxolotl changed the base branch from tracing/automanaged-tx to tracing/the-rest December 23, 2021 01:33

relaxolotl marked this pull request as draft December 23, 2021 03:58

relaxolotl marked this pull request as ready for review December 23, 2021 03:58

Base automatically changed from tracing/the-rest to master December 23, 2021 17:10

relaxolotl added 2 commits December 23, 2021 12:51

feat(tracing): Defer some transaction validation and allow creation o…

2721309

…f internal spans

oops, double set on operation

d27e0a5

relaxolotl force-pushed the tracing/lazy-validation branch from 5598343 to d27e0a5 Compare December 23, 2021 17:52

relaxolotl merged commit 7b168d8 into master Dec 23, 2021

relaxolotl deleted the tracing/lazy-validation branch December 23, 2021 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

relaxolotl commented Dec 18, 2021 •

edited

Loading

Swatinem Dec 21, 2021

flub Dec 21, 2021

relaxolotl Dec 23, 2021

flub Dec 21, 2021

relaxolotl Dec 23, 2021

flub Dec 21, 2021

relaxolotl Dec 23, 2021

flub Dec 21, 2021

relaxolotl Dec 23, 2021

flub Dec 21, 2021

relaxolotl commented Dec 23, 2021

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

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

Conversation

relaxolotl commented Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxolotl commented Dec 23, 2021

relaxolotl commented Dec 18, 2021 •

edited

Loading