barebones transaction work #618

relaxolotl · 2021-12-09T07:44:44Z

mostly a scratchpad of stuff being done to support bare bones transactions, plan is to break this up into smaller PRs

relaxolotl · 2021-12-09T07:46:53Z

oh no

Swatinem

this looks very nice so far!

Swatinem · 2021-12-09T10:04:45Z

include/sentry.h

+
+/**
+ * Sets the name of a transaction on a `transaction_context`.
+ * Returns 1 if the name provided is invalid (e.g. empty or nil).


Is this consistent with other similar APIs? I don’t quite remember if we even validate invalid values?

oh yeah, good catch. python and js merely warn and set the value to <unlabeled transaction>, so i'll fall back to that.

When I was experimenting with this in Rust, the server (relay?) would also default this. So the SDK is free to leave off the name completely (or make it empty) instead of defaulting it.

Swatinem · 2021-12-09T10:11:23Z

src/sentry_core.c

+    // TODO: does it matter that this is sampled on creation vs on send? can we
+    // just sample on send like other events?


good Q. I think since the sampling decision can also be set from the outside, it should be defined here?

potentially adding spans could become a no-op if sampled on creation? saves some cpu

loewenheim · 2021-12-09T13:59:08Z

src/sentry_core.c

+void
+sentry_transaction_finish(sentry_value_t tx)
+{
+    sentry_value_set_by_key(tx, "type", sentry_value_new_string("transaction"));


Why are we setting the type here?

good question; if you're wondering why it's set in finish of all places, i'd have to say that there's no real rhyme or reason behind it besides the idea that the transactions shouldn't be treated like a regular event (ie require checking of the type field) elsewhere in the code after their creation. that's likely a naive assumption given the lack of typing we have for transactions, though. now that i've written this out, i might just set this field during initialization to be safe.

if you're wondering why this field is getting set at all (and why it isn't being set for events), it's the discriminant in "events" (https://develop.sentry.dev/sdk/event-payloads/types/#eventtype). tl;dr SDKs send two types of events, error monitoring-related events and transactions. the former is the default and doesn't need an explicit type, the latter must have the type field set to transaction for it to be treated as such.

loewenheim · 2021-12-09T14:10:26Z

I don't quite understand how the more strongly typed API of sentry_tracing_stub.h is meant to interact with the JSONly typed API elsewhere.

relaxolotl · 2021-12-09T16:07:55Z

I don't quite understand how the more strongly typed API of sentry_tracing_stub.h is meant to interact with the JSONly typed API elsewhere.

apologies for the confusion; it was probably a mistake for me to commit this to begin with. it was probably really confusing to see these types and then discover that none of them are used anywhere in the code. i unfortunately haven't spent much time thinking about how i could integrate these types internally into the implementation while keeping the JSON-ly typed API that's currently exposed to users.

my mulling on this has terminated at "golly it would be a massive pain and waste of CPU cycles to be constantly converting between a sentry_value_t and a sentry_transaction_or_other_t". if you (or anybody else) have any ideas on how to tackle this challenge i'm all ears.

flub · 2021-12-13T16:23:49Z

examples/example.c

+        sentry_transaction_context_set_operation(
+            tx_ctx, "Short and stout here is my handle and here is my spout");


More of an api question, but i wonder if it makes sense to instead to always add this as a second argument to sentry_value_new_transaction_context("op", "name"), this would be similar to python's start_transaction(op, name). it was my understanding that you really want to have op filled in, if we do not want to make them obligatory we can still make them accept NULL for them.

also happy to discuss this on the notion page if you prefer

i don't have any particularly strong feelings on the matter - if it's more convenient to require an op and it lines up with the sort of behaviour we would like to encourage from users, then this can definitely be done. the change will most likely be made in #619.

i would like to call out the description on this PR, which is still applicable to the contents of its changes: the main intent of this PR is to simply expose the work i'm doing to the rest of the team, but none of it is really "PR-ready" from my POV. i can address the comments made on this diff and any discussion on parts of this that aren't already opened up in existing PRs are welcome, but given the genuine WIP nature of the changes on here some things are expected to be fairly rough (e.g. the documentation).

probably on me for making it possible to even see those parts of the diffs though...

i may be commenting on the wrong PRs. apologies, i've not been paying enough attention on what the various PRs are for

flub · 2021-12-13T16:28:20Z

include/sentry.h

+
+/**
+ * Constructs a new transaction context to be passed
+ * into `sentry_start_transaction`.


could we describe what the name parameter is? something like

*name* is the transaction name, this describes the transaction and can also be used to search for the transaction in sentry.

i'm a little reluctant to provide too much product guidance here as existing documentation tends to defer to documentation on docs.sentry.io on that front (ie sentry_value_new_event, sentry_value_new_message_event, sentry_value_new_exception, etc), preferring to mostly explain implementation-specific details. i'll admit that we can't really do the same for performance monitoring as its documentation isn't compiled in one easily referenceable location in the same way other types of messages are, making it difficult for us to do the same.

i've pushed an attempt to reach some sort of compromise between a full explanation and the approach used by existing documentation in #619.

flub · 2021-12-13T16:28:52Z

include/sentry.h

+    const char *name);
+
+/**
+ * Sets the name of a transaction on a `transaction_context`.


same, i'd like to describe what name is for

flub · 2021-12-13T16:30:53Z

include/sentry.h

+    sentry_value_t transaction_context, const char *name);
+
+/**
+ * Sets the operation of a transaction on a `transaction_context`.


similar, maybe describe what operation is?

flub · 2021-12-13T16:32:17Z

include/sentry.h

+ * When turned on, the transaction 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.


because sampled is such a confusing word, could we explicitly describe: 1: is sent to sentry; 0: is **not** sent to sentry?

flub · 2021-12-13T16:33:00Z

include/sentry.h

+    sentry_value_t transaction_context);
+
+/**
+ * Finishes a transaction.


is it now put on the transport to be sent to sentry? I'd add this in the description.

perhaps? i'm not sure about the details myself; this doesn't really do much of anything different from capture_event whose description is just Sends a sentry event.

so Finishes a transaction and sends it.?

flub · 2021-12-13T16:45:17Z

src/sentry_core.c

+    // TODO: does it matter that this is sampled on creation vs on send? can we
+    // just sample on send like other events?


potentially adding spans could become a no-op if sampled on creation? saves some cpu

src/sentry_value.c

tests/unit/test_tracing.c

relaxolotl · 2021-12-14T06:04:39Z

got started in creating some transaction-equivalents to envelope helpers that can also be found in other SDKs. the term "event" is starting to gain two different definitions though: the blanket "Event" that covers all types of items that may be stuffed into an envelope, and the "Event" that is a specific variant of the blanket "Event" which is one of those very items that can be placed in an envelope, and is a sibling to a Transaction.

…ransaction

…nsactions

…ransaction events

…ions

…action

…es-transaction

…ransaction

relaxolotl · 2021-12-18T03:04:36Z

now outdated; other PRs are now ahead of this work and i've switched to a different (local) scratchpad branch.

relaxolotl changed the base branch from master to tracing/set-trace-context December 9, 2021 07:44

relaxolotl force-pushed the tracing/barebones-transaction branch from ee11544 to f164c37 Compare December 9, 2021 07:48

Swatinem reviewed Dec 9, 2021

View reviewed changes

loewenheim reviewed Dec 9, 2021

View reviewed changes

relaxolotl force-pushed the tracing/set-trace-context branch from 955a459 to 148e3e1 Compare December 10, 2021 02:46

relaxolotl mentioned this pull request Dec 10, 2021

feat(tracing): Basic transaction context creation #619

Merged

relaxolotl force-pushed the tracing/barebones-transaction branch from f164c37 to c7b300c Compare December 10, 2021 20:09

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

relaxolotl force-pushed the tracing/barebones-transaction branch from c7b300c to a1e7cb8 Compare December 10, 2021 20:20

flub reviewed 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/barebones-transaction branch from a1e7cb8 to efe5801 Compare December 14, 2021 00:23

relaxolotl added 3 commits December 13, 2021 21:04

allow removal of the sampled field

02c58f5

feat(tracing): Support basic sampling of transactions

cae8d9d

better names for functions to avoid confusion

ffc13df

relaxolotl force-pushed the tracing/barebones-transaction branch 3 times, most recently from 7bc0497 to fbf4bd9 Compare December 14, 2021 02:31

relaxolotl changed the base branch from master to tracing/sampling-decision December 14, 2021 02:37

relaxolotl force-pushed the tracing/barebones-transaction branch from fbf4bd9 to 0a972a0 Compare December 14, 2021 02:44

add unit tests for sampling functions

5493073

relaxolotl force-pushed the tracing/sampling-decision branch 2 times, most recently from 5493073 to c8236ed Compare December 14, 2021 04:46

the rest

aa34743

relaxolotl force-pushed the tracing/barebones-transaction branch from 0a972a0 to aa34743 Compare December 14, 2021 05:58

Base automatically changed from tracing/sampling-decision to master December 15, 2021 20:42

relaxolotl added 17 commits December 15, 2021 16:08

Merge remote-tracking branch 'origin/master' into tracing/barebones-t…

5b91c4f

…ransaction

fix up names

6c8e362

include sent_at header in transactions

265beaf

have a helper that tells you if an event is a transaction or not

4b7305b

wip

75c6dd3

feat(tracing): Introduce a helper that identifies events that are tra…

2f9f62c

…nsactions

forgot header

1bb238d

missed a spot

662159c

feat(tracing): Restrict sentry_capture_event so it only sends non-t…

0d90d95

…ransaction events

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

5efdf0f

feat(tracing): Allow manual creation and sending of spanless Transact…

b6157f8

…ions

rename sentry_start_transaction to sentry_transaction_start

5154145

brush up docstrings

863e7c1

Merge branch 'tracing/tx-differentiator' into tracing/barebones-trans…

92ae7fd

…action

Merge branch 'tracing/opinionated-capture-event' into tracing/barebon…

9c482cc

…es-transaction

Merge branch 'tracing/base-envelope-support' into tracing/barebones-t…

79d6d6a

…ransaction

Merge branch 'tracing/the-rest' into tracing/barebones-transaction

8c7af3f

relaxolotl closed this Dec 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

barebones transaction work #618

barebones transaction work #618

relaxolotl commented Dec 9, 2021

relaxolotl commented Dec 9, 2021

Swatinem left a comment

Swatinem Dec 9, 2021

relaxolotl Dec 9, 2021

Swatinem Dec 9, 2021

Swatinem Dec 9, 2021

flub Dec 13, 2021

loewenheim Dec 9, 2021

relaxolotl Dec 9, 2021

loewenheim commented Dec 9, 2021

relaxolotl commented Dec 9, 2021

flub Dec 13, 2021

relaxolotl Dec 14, 2021

relaxolotl Dec 14, 2021

flub Dec 14, 2021

flub Dec 13, 2021

relaxolotl Dec 14, 2021

flub Dec 13, 2021

flub Dec 13, 2021

flub Dec 13, 2021

flub Dec 13, 2021

relaxolotl Dec 14, 2021

flub Dec 14, 2021

flub Dec 13, 2021

relaxolotl commented Dec 14, 2021

relaxolotl commented Dec 18, 2021

		// TODO: does it matter that this is sampled on creation vs on send? can we
		// just sample on send like other events?

		sentry_transaction_context_set_operation(
		tx_ctx, "Short and stout here is my handle and here is my spout");

barebones transaction work #618

barebones transaction work #618

Conversation

relaxolotl commented Dec 9, 2021

relaxolotl commented Dec 9, 2021

Swatinem left a 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

loewenheim commented Dec 9, 2021

relaxolotl commented Dec 9, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relaxolotl commented Dec 14, 2021

relaxolotl commented Dec 18, 2021