sentry-native icon indicating copy to clipboard operation
sentry-native copied to clipboard

Question: sentry__get_span_or_transaction

Open tiny-dancer opened this issue 3 years ago • 5 comments

Leveraging the native sdk qt application and attempting to provide integrations into a variety of areas, one included being http. When attempting to provide a decoupled http implementation (not passing tx/span all the way down the call stack), I wasn't able to find a way in the native sdk to retrieve the current scope transaction/span.

Attempting to implement something similar to: https://github.com/getsentry/sentry-javascript/blob/master/packages/node/src/integrations/http.ts#L152 for qnetworkaccessmanager however it appears the relevant function is only enabled during unit tests: https://github.com/getsentry/sentry-native/blob/master/src/sentry_scope.c#L248

Is this expected? Am I on the right track?

Thanks!

tiny-dancer avatar Oct 12 '22 00:10 tiny-dancer

You are on the right track, but the functions you are looking for are currently not exposed to the sentry-native API. But this is more by accident than by design. We think retrieving txn/span from the scope should be possible.

supervacuus avatar Oct 12 '22 14:10 supervacuus

@supervacuus thanks for the quick follow up. Is this on the upcoming radar? Happy to take a stab at a first time contribution if you see it as a reasonable first time contribution and helpful to do so.

tiny-dancer avatar Oct 12 '22 15:10 tiny-dancer

We have a few other things in our pipeline, but it is not a significant change (so we can probably fit it in somewhere). But if you need it very soon (eg. this week), then yes, this would be a reasonable first-time contribution.

supervacuus avatar Oct 13 '22 13:10 supervacuus

Ok great, sounds like it wouldn't be that far off in the distance and not an immediate need here. 50/50 if i get to it in the next week otherwise will wait.

tiny-dancer avatar Oct 14 '22 23:10 tiny-dancer

Hi again, I just wanted to let you know that we had a short discussion about what changes to the API would be necessary to expose the span/txn from the scope.

While simply exposing sentry__scope_get_span_or_transaction() as a public API would be the easiest, it would introduce a long-term issue that we'd like to prevent and probably was the main reason that it wasn't introduced initially.

As you might have seen in the span/txn API header docs, those values need to be manually synchronized on the client side if it accesses them from multiple threads. In addition, in the case of span/txn stored in the scope, the client needs to manage accesses where the scope and span must change transactionally. While you might not need access from multiple threads, we need to consider use cases like these for sentry-native in general.

One idea to cleanly solve this would be to duplicate the span/txn API for span/txn in the scope and take care of synchronization this way. This would mean you'd never get a reference to scope/txn in the scope but rather have functions that work on them implicitly.

So, for example, instead of

void sentry_transaction_set_tag(
    sentry_transaction_t *transaction, const char *tag, const char *value)

you would have

void sentry_scope_transaction_set_tag(const char *tag, const char *value)

Of course, there will still be some functions that expose child spans to the client side (which , again, needs synchronization in a multi-threaded scenario), but the idea would be never to reveal the root value within the scope.

Would this make sense in your setup? What kind of mutations would you need to operate on the span/txn within the scope?

I will be unavailable during this week but will have time to react to responses next week.

supervacuus avatar Oct 23 '22 22:10 supervacuus