Support performance

[Tyrrrz]

502 (not sure if this will enable release health as well?)

[Tyrrrz]

I know the interface is really exposed here and ideally we would want to add some methods to make things nicer (i.e. set timestamps automatically, etc.).

For now, I want to wire this on Sentry.AspNetCore side first. I have never done that, any guidance on how should I approach this? @bruno-garcia @lucas-zimerman

[Tyrrrz]

@bruno-garcia

Sentry docs say:

A Transaction is basically a Span combined with an Event.

Is it true in regards to our event structure? Or should it just implement IScope?

[Tyrrrz]

As there any additional options that need to be added into SentryOptions for this?

Looks like Java SDK added these. Anything else?

[lucas-zimerman]

@bruno-garcia

Sentry docs say:

A Transaction is basically a Span combined with an Event.

Is it true in regards to our event structure? Or should it just implement IScope?

True, the transaction it's basically 'IScope' with some extras (a list of pans, StartTimestamp and some functions for creating a child). Now I'm unsure if the transaction is going to be consumed by the existing event processors + before send the event, like some platforms.

[lucas-zimerman]

If it helps https://github.com/lucas-zimerman/ContribSentryDotNet/blob/ref/better_tracing/ContribSentry/Internals/SpanStatus.cs Here's the list of Span/Transaction status (The exception part wasn't defined by the Doc so I was experimenting with it)

[lucas-zimerman]

As there any additional options that need to be added into SentryOptions for this?

Looks like Java SDK added these. Anything else?

for each transaction, I was adding a "transaction Breadcrumb", if you're also going to add the breadcrumb for every transaction, it would be nice to have an option to disable the transaction breadcrumb

Breadcrumb example: SentrySdk.AddBreadcrumb(tracing.EventId.ToString(), "sentry.transaction");

[Tyrrrz]

@lucas-zimerman does your project also have AspNetCore integration for transactions?

[lucas-zimerman]

@lucas-zimerman does your project also have AspNetCore integration for transactions?

There's no middleware for it but it should work just fine if you manually call the StartTransaction where you would like to measure the performance.

All you need is an AsyncLocal control and a way for controlling when it starts and when it ends (with or without any error).

        public Task StartTransaction(string name, string method, Action<ISentryTracing> action)
            => Task.Run(() =>
            {
                ISentryTracing tracing = DisabledTracing.Instance;
                if(CurrentTransaction.Value is null)
                {
                    CurrentTransaction.Value = new SentryTracing(name, method);
                    tracing = CurrentTransaction.Value;
                }
                try
                {
                    action(tracing);
                    tracing.Finish();
                }
                catch(Exception ex)
                {
                    tracing.Finish(ex);
                    throw;
                }
            });

https://github.com/lucas-zimerman/ContribSentryDotNet/blob/d049f3fe630ffead4636c33b30c0dca7315cecae/ContribSentry/Internals/TransactionWorker.cs#L35

[lucas-zimerman]

502 (not sure if this will enable release health as well?)

Only Performance, release health is another pack of new classes.

[Tyrrrz]

@bruno-garcia regarding aspnetcore middleware, do we programmatically add one before endpoint routing middleware and then measure the execution time of await next()? Or something more clever?

[bruno-garcia]

@bruno-garcia regarding aspnetcore middleware, do we programmatically add one before endpoint routing middleware and then measure the execution time of await next()? Or something more clever?

That sounds like it would work yeah

[Tyrrrz]

@bruno-garcia do we create some spans automatically for nested operations using the middleware? Or just let the user do it themselves.

[bruno-garcia]

@Tyrrrz any outgoing HTTP request, SQL queries would eventually need to become spans, and automatically. We'll get there eventually though, for now if we can instrument some areas like Controller action invocation, and HTTP requests only would bea great start

[Tyrrrz]

@bruno-garcia where should the current transaction be kept? Should it be kept on the current scope? Or should we create a new AsyncLocal for transaction? Maybe something like ConfigureTransaction that incidentally calls ConfigureScope?

[Tyrrrz]

@bruno-garcia @lucas-zimerman can spans be infinitely nested within other spans? or is it just the transaction that can have spans?

[lucas-zimerman]

@bruno-garcia @lucas-zimerman can spans be infinitely nested within other spans? or is it just the transaction that can have spans?

Each Span could have N childrens Example

Not sure if the Doc specifies some limit but I'd guess the only limit is the request size.

[bruno-garcia]

Transaction class has a list of Spans, they are linked in Sentry with their Ids.. the SDK doesn't hold a tree structure.

The Scope can hold a Transaction. This was done in the Java SDK, you can peek the implementation there.

The Ruby PR is less than a 1k lines: https://github.com/getsentry/sentry-ruby/pull/1108

If the docs are not very clear, we need to improve the docs.

[bruno-garcia]

We also need to consider that AsyncLocal will need to change later, to better serve Desktop and Mobile.

[Tyrrrz]

@bruno-garcia

Transaction class has a list of Spans, they are linked in Sentry with their Ids.. the SDK doesn't hold a tree structure.

Ok, so the spans can also have spans inside, but the class representation itself doesn't keep track of the list, right?

The Scope can hold a Transaction. This was done in the Java SDK, you can peek the implementation there.

Ok, cool.

If the docs are not very clear, we need to improve the docs.

If you look at the transaction/span docs, they are all over the place. The list of fields does not match the example at the end. In docs it says event_id while in reality it's span_id. The example on transaction docs actually show the example from span docs. Etcetera.

[Tyrrrz]

Also, @bruno-garcia

IScope and Scope already have Transaction, which is a string. This looks like it corresponds to Transaction.Name. Should we remove this and replace with a transaction? Or do something else?

Looking at Java SDK, it looks like that's exactly what happened there:

[bruno-garcia]

If you look at the transaction/span docs, they are all over the place. The list of fields does not match the example at the end. In docs it says event_id while in reality it's span_id. The example on transaction docs actually show the example from span docs. Etcetera.

Would be nice to get this fixed. Do you mind opening a PR?

[bruno-garcia]

Today the Transaction field is used by the error product. In Java, it's still there for the Event:

https://github.com/getsentry/sentry-java/blob/3ed2ff105dd246cbf0242e802543c710bcb14a0b/sentry/src/main/java/io/sentry/SentryEvent.java#L89

As I said above, there are two classes: SentryEvent and Transaction. From Sentry's point of view, both are Events.

[Tyrrrz]

Today the Transaction field is used by the error product. In Java, it's still there for the Event:

https://github.com/getsentry/sentry-java/blob/3ed2ff105dd246cbf0242e802543c710bcb14a0b/sentry/src/main/java/io/sentry/SentryEvent.java#L89

As I said above, there are two classes: SentryEvent and Transaction. From Sentry's point of view, both are Events.

I see. But it's removed on the Scope though?

[Tyrrrz]

@bruno-garcia

... From Sentry's point of view, both are Events.

I'm confused what do you mean by this, because it seems that Sentry docs discriminate between Events and Transactions as different envelope items:

[lucas-zimerman]

oof sorry D:

[Tyrrrz]

Another question: what is Sentry trace header for?

[rhcarvalho]

@bruno-garcia

... From Sentry's point of view, both are Events.

I'm confused what do you mean by this, because it seems that Sentry docs discriminate between Events and Transactions as different envelope items.

Let me try to help Bruno and clarify.

In memory, spans build-up a conceptual tree of timed operations. We call the whole span tree a transaction. So one can think of the transaction as either the whole tree or the root span of the tree (in inheritance terms, Transaction "is a (subclass of)" Span).

Over the wire, transactions are serialized to JSON as an augmented Event, and sent as envelopes. The different envelope types are for optimizing ingestion (so we can route "transaction events" differently than other events, mostly "error events").

In the Sentry UI, you can use Discover to look at all events regardless of type, and the Issues and Performance sections to dive into errors and transactions, respectively.

I hope that helps :)

[rhcarvalho]

Another question: what is Sentry trace header for?

The header is for trace propagation. https://develop.sentry.dev/sdk/unified-api/tracing#header-sentry-trace

Long story short, the SDK uses the header to continue traces from upstream services (incoming HTTP requests), and to propagate tracing information to downstream services (outgoing HTTP requests).

[Tyrrrz]

@bruno-garcia

... From Sentry's point of view, both are Events.

I'm confused what do you mean by this, because it seems that Sentry docs discriminate between Events and Transactions as different envelope items.

Let me try to help Bruno and clarify.

In memory, spans build-up a conceptual tree of timed operations. We call the whole span tree a transaction. So one can think of the transaction as either the whole tree or the root span of the tree (in inheritance terms, Transaction "is a (subclass of)" Span).

Over the wire, transactions are serialized to JSON as an augmented Event, and sent as envelopes. The different envelope types are for optimizing ingestion (so we can route "transaction events" differently than other events, mostly "error events").

In the Sentry UI, you can use Discover to look at all events regardless of type, and the Issues and Performance sections to dive into errors and transactions, respectively.

I hope that helps :)

I see, yes that clarified a lot, thanks.

Do you differentiate between Event and Transaction at the SDK level (in the Go implementation) or do you just attach a Span to the current scope and then decide whether to encode it as event or transaction based on whether the Span is set?

[Tyrrrz]

Another question: what is Sentry trace header for?

The header is for trace propagation. https://develop.sentry.dev/sdk/unified-api/tracing#header-sentry-trace

Long story short, the SDK uses the header to continue traces from upstream services (incoming HTTP requests), and to propagate tracing information to downstream services (outgoing HTTP requests).

Thanks, that makes sense!

[rhcarvalho]

Good questions, thanks @Tyrrrz

Do you differentiate between Event and Transaction at the SDK level (in the Go implementation) or do you just attach a Span to the current scope and then decide whether to encode it as event or transaction based on whether the Span is set?

The choice is not between event or transaction, but event or nothing -- we don't send spans on their own.

T -
    \
    S1
     |- S11
    S2

Calling Finish on any span S* sets their end time. Calling Finish on the transaction / root span T sets its end time AND sends a "transaction event" (and event with Event.Type = "transaction" and other fields filled in) to Sentry.

Example in Go of how a root span / transaction gets converted into an Event: https://github.com/getsentry/sentry-go/blob/111eae57bd5a5953ed4df47d7ccb7d0d8cf8b3d4/tracing.go#L311-L338

---

Disclaimer: there is no concept of inheritance in Go (nor classes), so it is implemented different than JS and Python, for example.

Disclaimer 2: personally, I find it harder to write generic middleware / instrumentation code if it needs to decide whether to start a transaction or a span (depending on the pre-existence of a parent / span tree somewhere).

For the above reasons, in Go, there is only Span and StartSpan. Starting a span in an empty context makes it a "transaction" when it comes the time to serialize the tree as an Event (when the Span.Finish method is called). There is no Transaction type.

Span.Finish of the root span converts the span tree into an Event (as I linked above) and calls the existing CaptureEvent method to send the transaction event to Sentry. The transports deal with the envelope format and figuring out the right endpoint before sending out to Sentry based on Event.Type.

While the span is ongoing, it is stored in a Go-specific Context type, which plays a role similar to Scope, and was a more idiomatic choice. Unless there's something similarly idiomatic for .NET, the expectation is that the current span stays in the Scope.

[kanadaj]

That might actually be a direction worth going for. The current state of Python instrumentation makes it so tracing code errors out if there isn't a transaction already, which is a bit of a pain when sharing code between a web end point and CLI, and that's probably something we should avoid in .NET

[untitaker]

tracing code errors out if there isn't a transaction already

spans are discarded when a transaction is not open, which is not exactly erroring out

Starting a span in an empty context makes it a "transaction" when it comes the time to serialize the tree as an Event (when the Span.Finish method is called). There is no Transaction type.

This has the disadvantage that every db span will suddenly become its own transaction if there's no wrapping transaction for the endpoint. This is billing relevant.

[bruno-garcia]

tracing code errors out if there isn't a transaction already

spans are discarded when a transaction is not open, which is not exactly erroring out

I'd like to stick to this behavior (which was used in Java too). I understand @rhcarvalho's concern about having to diferentiate a startSpan and startTransaction but in a middleware, of our auto instrumentation integration, we can define that: Here we start a new transaction (not a span). If there's a transaction already open, we can then just add a span but that's a fallback scenario, unlikely to happen. I'm thinking here when do you have something that creates a Sentry transaction in a ASP.NET Core requests that runs before we hadnle the incoming HTTP request. i can't think of a legit use case for that. Even not doing anything if a transaction exists is OK (don't create a span, don't acknowledge that use case)

Starting a span in an empty context makes it a "transaction" when it comes the time to serialize the tree as an Event (when the Span.Finish method is called). There is no Transaction type.

This has the disadvantage that every db span will suddenly become its own transaction if there's no wrapping transaction for the endpoint. This is billing relevant.

Goes wiht the first comment, we should be explicit here if a transaction is being started or it's a span only.

[Tyrrrz]

@bruno-garcia regarding Transaction inheriting most of Event's fields:

• Does transaction also have exception? If yes, how does it work/ how it gets propagated?
• I imagine fields that are on scope get copied onto the transaction, but what about fields that are on event but not on scope? For example, on event, stuff like event.Release etc are set from event processors, but I assume they won't be called on transactions? or will they?

[aleksihakli]

Hey! Just wanted to pop in and say that this PR and performance monitoring for .NET Core would be a very nice thing to have upstream. Is there anything you need help with in upstreaming this and getting a new release with the performance addon included?

[bruno-garcia]

@aleksihakli thanks for chiming in. We're planning to get this merged and make a release soon.

Would you be willing to try a preview? We'll release a 3.0.0-alpha.X with this code as soon as @Tyrrrz merges this

[aleksihakli]

@aleksihakli thanks for chiming in. We're planning to get this merged and make a release soon.

Would you be willing to try a preview? We'll release a 3.0.0-alpha.X with this code as soon as @Tyrrrz merges this

Hey Bruno! Sure, I think we have some .NET (Core) projects that I could try the alpha version in and give feedback on :) Once the PR is upstreamed and possibly released to package repository I'll try and get it running against the Sentry SaaS version. We also have a recent version of Sentr on-premises running if testing against both distributions is desirable.

[bruno-garcia]

@aleksihakli thanks for chiming in. We're planning to get this merged and make a release soon. Would you be willing to try a preview? We'll release a 3.0.0-alpha.X with this code as soon as @Tyrrrz merges this

Hey Bruno! Sure, I think we have some .NET (Core) projects that I could try the alpha version in and give feedback on :) Once the PR is upstreamed and possibly released to package repository I'll try and get it running against the Sentry SaaS version. We also have a recent version of Sentr on-premises running if testing against both distributions is desirable.

That's awesome! Thanks. One note for Sentry on-premise. In the 3.0.0 of this SDK we moved to the /envelope endpoint, so Sentry on-prem must be v20.6 or higher.

But testing only on SaaS is fine. It'sn really about how does it work for you, if you have any issues, and ultimately, does it add value? Does it help you improve the experience for your customers by identifying slowdowns and giving you insight to improve them.

We're online on Sentry's Discord if you'd like to chat. On #dotnet

[codecov-io]

Codecov Report

Merging #633 (eb37814) into main (42368a1) will decrease coverage by 2.38%. The diff coverage is 52.01%.

@@            Coverage Diff             @@
##             main     #633      +/-   ##
==========================================
- Coverage   79.91%   77.53%   -2.39%     
==========================================
  Files         153      162       +9     
  Lines        4814     5212     +398     
  Branches     1251     1347      +96     
==========================================
+ Hits         3847     4041     +194     
- Misses        608      784     +176     
- Partials      359      387      +28     

Impacted Files	Coverage Δ
src/Sentry/Internal/Extensions/JsonExtensions.cs	65.45% <0.00%> (-3.78%)	:arrow_down:
src/Sentry/Internal/Polyfills.cs	66.66% <0.00%> (-8.34%)	:arrow_down:
src/Sentry/Protocol/SentryTraceHeader.cs	0.00% <0.00%> (ø)
src/Sentry/Protocol/Span.cs	0.00% <0.00%> (ø)
src/Sentry/TraceSamplingContext.cs	0.00% <0.00%> (ø)
src/Sentry/Protocol/SpanRecorder.cs	20.00% <20.00%> (ø)
src/Sentry/Extensibility/DisabledHub.cs	83.33% <33.33%> (-10.00%)	:arrow_down:
src/Sentry/Extensibility/HubAdapter.cs	84.61% <33.33%> (-6.69%)	:arrow_down:
src/Sentry/SentrySdk.cs	89.36% <33.33%> (-3.83%)	:arrow_down:
src/Sentry.AspNetCore/SentryMiddleware.cs	81.05% <44.44%> (-14.60%)	:arrow_down:
... and 29 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 42368a1...61085a4. Read the comment docs.

[Tyrrrz]

Can't understand what failed windows build.

[Tyrrrz]

Thanks @aleksihakli

[bruno-garcia]

@aleksihakli thanks for helping out with reviewing!

[aleksihakli]

Took a look and saw that 3.0.0-alpha.7 is now available in package repositories.

I'm not sure if I have the time to test drive this before the Christmas holidays, but we'll most probably have the chance to start testing the performance side of things early next year in different environments.

Super excited to get this integrated upstream, thanks for the effort folks! Ever since OpBeat was acquired and merged into Elastic APM I've been looking for other viable performance tracking options and think that Sentry is an increasingly exciting option for projects of all size and has a good model with the FOSS and SaaS deployments available. Keep up the great work.

[aleksihakli]

Added the SDK to a minimal project and can confirm that the service is indeed sending metrics with 3.0.0-alpha.10 which seems good.

What kind of setups should be tested for a typical .NET Core web project? Entity Framework, API calls, library calls, anything interesting in there?

[bruno-garcia]

Thanks for testing @aleksihakli

Would you want to discuss this on Discord instead of this PR? It's been merged, I often skip reading messages on merged PR or close issues.

https://discord.gg/PXa5Apfe7K

There's a channel #dotnet there 👍