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

Jump to bottom

feat(tracing): Groundwork to add tracing context to all events #617

Merged
merged 6 commits into from
Dec 13, 2021
Merged

feat(tracing): Groundwork to add tracing context to all events #617

merged 6 commits into from
Dec 13, 2021

Conversation

relaxolotl
Copy link
Contributor

@relaxolotl relaxolotl commented Dec 9, 2021 •
edited
Loading

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 in this PR.

The focus of this PR 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 right now are basically nonexistent.

relates to #601

@relaxolotl relaxolotl requested a review from a team December 9, 2021 07:27
@relaxolotl
Copy link
Contributor Author

i'll fix the unused parameter tomorrow

Swatinem
Swatinem approved these changes Dec 9, 2021
View reviewed changes
src/sentry_scope.c Outdated Show resolved Hide resolved
src/sentry_scope.h
// and to avoid a conflict with the existing transaction field this is named
// span. Whenever possible, `transaction` should pull its value from the
// `name` property nested in this field.
sentry_value_t span;
Copy link
Member

Choose a reason for hiding this comment

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

The description is very nice! Maybe name this root_span or something? Not sure what would make this less confusing.
Also not sure if you want to save the innermost span somewhere as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

truth be told i'm actually not the biggest fan of calling these spans as there are subtle differences between a transaction and a span. the nice thing about using span here is mostly that it's consistent with the naming scheme used by other SDKs (see https://github.com/getsentry/sentry-python/blob/dd0efc08414ee2ef1a5f22d2cc4e243b54a1b455/sentry_sdk/scope.py#L81-L83).

given that, i'm reluctant to use root_span because in theory, this "span" should be a transaction which means it actually should contain some sort of pointer to a span tree(s). there have been suggestions to call this something like transaction_object, which makes it less of a misnomer at the cost of giving up consistency. thoughts?

i think the question you have about the innermost span might be indirectly answered by the second paragraph: the stored "span", aka the transaction should have enough info to find any given span that branches off of it if needed. i'll admit however that i don't have a very good vision of how span tree construction and management will look like yet, and i'm leaving that as a challenge to be solved a little later.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on transaction_object, why not. The comment can mention that other SDKs call this simply span it is remains greppable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's also a setter on scope that the spec explicitly requires to be named set_span: https://develop.sentry.dev/sdk/performance/#scope-changes

given that this setter is meant to be internal and this suggested change's intent is to make things easier for us internally, i'd like to do this: i'm going to merge this as-is while it still uses the terminology as required by the spec and as used by all of the other SDKs. a follow-up PR can be opened up which introduces this rename of span to transaction_object in the scope in addition to the introduction of a set_transaction_object() helper to mirror the rename. we can then assess whether the rename works or makes sense or not in there.

src/sentry_tracing.c Show resolved Hide resolved
Base automatically changed from tracing/internal-uuid to master December 10, 2021 20:15
flub
flub approved these changes Dec 13, 2021
View reviewed changes
src/sentry_scope.h
// and to avoid a conflict with the existing transaction field this is named
// span. Whenever possible, `transaction` should pull its value from the
// `name` property nested in this field.
sentry_value_t span;
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on transaction_object, why not. The comment can mention that other SDKs call this simply span it is remains greppable.

@relaxolotl relaxolotl merged commit f2d1d4b into master Dec 13, 2021
@relaxolotl relaxolotl deleted the tracing/set-trace-context branch December 13, 2021 23:56
nunomluz pushed a commit to mystaff/sentry-native that referenced this pull request Dec 24, 2021
@pull @relaxolotl
@loewenheim @getsentry-bot @Swatinem
[pull] master from getsentry:master (#14)
f86afdd 
* 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>
nunomluz added a commit to mystaff/sentry-native that referenced this pull request Dec 27, 2021
@nunomluz @torarnv
@Swatinem @encounter @getsentry-bot @bschatt @BYK @GenuineAster @janisozaur @Gerhard-Herbert @andrei-mu @past-due @rpadaki @mjvankampen @NattyNarwhal @ladislavmacoun @Mixaill @pull @relaxolotl @loewenheim
Upstream sentry-native (does not update crashpad and breakpad refs) (#15
ce34056 
)

* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flub flub flub approved these changes

@Swatinem Swatinem Swatinem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@relaxolotl @flub @Swatinem