Support performance #633

Tyrrrz · 2020-11-30T19:19:32Z

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

Tyrrrz · 2020-11-30T19:29:12Z

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 · 2020-11-30T19:32:14Z

@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 · 2020-11-30T19:36:02Z

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

Looks like Java SDK added these. Anything else?

lucas-zimerman · 2020-11-30T19:37:51Z

@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 · 2020-11-30T19:40:28Z

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 · 2020-11-30T19:45:53Z

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 · 2020-11-30T19:55:30Z

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

lucas-zimerman · 2020-11-30T20:03:45Z

@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 · 2020-11-30T20:35:47Z

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

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

bruno-garcia

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

Let's focus on Performance only for now.

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

Yeah technically a Transaction is a SentryEvent but we don't want BeforeSend to run for example (and hopefully no EventProcessors too), but we might have to let EventProcessors run if we need to align with other SDKs that did that (by accident).

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

Those two. We can add that later, lets go without sampling in this PR to make smaller chunks but keep that in mind.

bruno-garcia · 2020-11-30T21:53:31Z

src/Sentry/Protocol/Span.cs

+        public bool IsSampled { get; set; }
+
+        private ConcurrentDictionary<string, string>? _tags;
+        public IReadOnlyDictionary<string, string> Tags => _tags ??= new ConcurrentDictionary<string, string>();


I assume concurrency here might be more of an issue so this approach of instantiating on the getter won't work well here.

We might need to synchronize in order to make sure concurrent calls to get get the same instance. Maybe a double check lock will do

So Spans have tags? I was thinking that only Transactions could have tags

Maybe just eagerly initialize the instance? It will probably be a smaller overhead than a lock.

one will incur overhead on each new span due to allocating the list then GC'ing it. The other one will cost when accessing it only and only the first time due to the double check lock.

Without measuring we're just guessing, the the transaction I'm happy with always having the list there since more often than not we'll use it, for individual Spans, I don't know

The other one will cost when accessing it only and only the first time due to the double check lock.

Maybe I misunderstood your suggestion, but how will that work? From my understanding, the lock has to be on the getter, which would mean it's acquired/checked every time the property is accessed.

https://en.wikipedia.org/wiki/Double-checked_locking

@lucas-zimerman the docs say so, but I also found the docs to be incredibly unreliable too, so...
https://develop.sentry.dev/sdk/event-payloads/span/

If the docs are not good, we should fix it. Please open a PR there or at least raise a flag (open an issue asking for clarification).

bruno-garcia · 2020-11-30T21:55:09Z

src/Sentry/Protocol/Transaction.cs

+
+namespace Sentry.Protocol
+{
+    public class Transaction : ISpan, IJsonSerializable


Transaction is actually a whole Event :(

That's how it is today in Sentry, basically an event with a contexts.trace field and spans

We can try to go with ISpan for now and see what it looks like.

What is contexts.trace actually? I've seen it referenced before but still not sure.

Also, "transaction is actually a whole Event", does that affect how we should design the class? We probably can't make use of inheritance anyway due to manual serialization. I'm thinking of doing public class Transactions : ISpan, IScope, would that be enough?

we could inherit and call the base serializer to add some stuff and the Transaction one to add the rest.

In Java it was split in 2 parts, SentryEvent (current one) and Transaction, and both inherit from some BaseEvent.

we could inherit and call the base serializer to add some stuff and the Transaction one to add the rest.

This will be messy because it will have to add serialization logic before the writer.WriteEndObject().

we could call a protected virtual AddMoreStuff(writer) and implement this in the derived type

src/Sentry/Protocol/ISpan.cs

Tyrrrz · 2020-11-30T23:15:41Z

@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 · 2020-11-30T23:30:31Z

@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 · 2020-11-30T23:35:20Z

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

bruno-garcia · 2020-11-30T23:52:29Z

@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 · 2020-12-01T17:40:30Z

@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 · 2020-12-01T17:50:43Z

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

lucas-zimerman · 2020-12-01T17:54:27Z

@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 · 2020-12-01T17:58:16Z

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: getsentry/sentry-ruby#1108

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

bruno-garcia · 2020-12-01T17:59:13Z

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

Tyrrrz · 2020-12-01T18:07:42Z

@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 · 2020-12-01T18:13:57Z

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 · 2020-12-01T21:25:37Z

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?

src/Sentry.AspNetCore/ScopeExtensions.cs

src/Sentry.AspNetCore/SentryMiddleware.cs

codecov-io · 2020-12-14T21:13:20Z

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%)`	⬇️
src/Sentry/Internal/Polyfills.cs	`66.66% <0.00%> (-8.34%)`	⬇️
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%)`	⬇️
src/Sentry/Extensibility/HubAdapter.cs	`84.61% <33.33%> (-6.69%)`	⬇️
src/Sentry/SentrySdk.cs	`89.36% <33.33%> (-3.83%)`	⬇️
src/Sentry.AspNetCore/SentryMiddleware.cs	`81.05% <44.44%> (-14.60%)`	⬇️
... 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 · 2020-12-14T21:26:30Z

Can't understand what failed windows build.

aleksihakli

C# and .NET aren't my strongest skills but a few notes from here and there. The PR looks clean and good as a whole, good job!

src/Sentry/Internal/Extensions/StringExtensions.cs

aleksihakli · 2020-12-15T13:12:52Z

src/Sentry/Protocol/Envelopes/EnvelopeItem.cs

+        /// </summary>
+        public static EnvelopeItem FromTransaction(Transaction transaction)
+        {
+            var header = new Dictionary<string, object?>(StringComparer.Ordinal)


Maybe that should be documented as a TODO now, if it is not yet included but could be added to the implementation later on?

test/Sentry.Tests/Protocol/TransactionTests.cs

Tyrrrz · 2020-12-15T14:35:18Z

Thanks @aleksihakli

bruno-garcia

There are a few things we can take on a new PR, but this is good to merge and put out on a new preview!

bruno-garcia · 2020-12-15T23:48:18Z

CHANGELOG.md

@@ -12,6 +14,7 @@
 * Add `SentryScopeStateProcessor` #603
 * Add net5.0 TFM to libraries #606
 * Add more logging to CachingTransport #619
+* Bump Microsoft.Bcl.AsyncInterfaces to 5.0.0 #618


It's on the line below (it got some ` on another PR)

Suggested change

* Bump Microsoft.Bcl.AsyncInterfaces to 5.0.0 #618

bruno-garcia · 2020-12-16T02:22:31Z

src/Sentry/Extensibility/DisabledHub.cs

+        /// Returns a dummy transaction.
+        /// </summary>
+        public Transaction CreateTransaction(string name, string operation) =>
+            new Transaction(this, null);


We should have a singleton NoOp instance here instead.
Having a disabled hub means we should reduce the instrumentations overhead

bruno-garcia · 2020-12-16T02:25:52Z

src/Sentry/Internal/Hub.cs

+                trans.Sdk.AddPackage(protocolPackageName, nameAndVersion.Version);
+            }
+
+            ConfigureScope(scope => scope.Transaction = trans);


I believe by default we won't do this. The snippets I saw from JS require the caller to decide whether the transaction should be bound to the scope or not.

For now this looks good and lets try like this.

bruno-garcia · 2020-12-16T02:27:46Z

src/Sentry/Internal/MainSentryEventProcessor.cs

-            if (@event.Sdk != null)
+            // SDK Name/Version might have be already set by an outer package
+            // e.g: ASP.NET Core can set itself as the SDK
+            if (@event.Sdk.Version == null && @event.Sdk.Name == null)


Are transactions going through this at the end? Doesn't seem like it since this take SentryEvent.

So transactions will always be reported as sentry.dotnet?

Yeah... The SDK is set separately when creating the transaction right now. Transactions also don't get all the other default metadata either, which is populated by event processors.

bruno-garcia · 2020-12-16T02:29:47Z

src/Sentry/Protocol/Context/Trace.cs

+        /// </summary>
+        public static Trace FromJson(JsonElement json)
+        {
+            var spanId = json.GetPropertyOrNull("span_id")?.Pipe(SpanId.FromJson) ?? SpanId.Empty;


Invalid json blobs will result in broken traces being sent (empty string ids etc)

I think we should be returning nullable and dealing with it.

bruno-garcia · 2020-12-16T02:39:34Z

src/Sentry/Protocol/SpanRecorder.cs

+        {
+            lock (_lock)
+            {
+                return _spans;


There's exclusive access to just return the reference, which now can be access outside of any locks by the caller which can iterate over it while another thread is adding to it, which would throw an exception.

We need a new list, with the current items:

Suggested change

return _spans;

return new List<Span>(_spans);

bruno-garcia · 2020-12-16T02:43:32Z

src/Sentry/Protocol/SpanRecorder.cs

+        /// </summary>
+        public void Add(Span span)
+        {
+            lock (_lock)


Taking a lock to add any and all Spans can possibly create contention if we have a bunch of spans getting added to the same transaction.

Lets revisit this later to add a capped list that won't require exclusive access on all Add

Maybe just a Interlocked.Increment so we keep track of the count of items in the _span and using ConcurrentBag would be an improvement. But we discuss/measure things later, this will do for now.

bruno-garcia · 2020-12-16T02:52:22Z

src/Sentry/Protocol/Transaction.cs

+        }
+
+        /// <inheritdoc />
+        public SentryLevel? Level { get; set; }


This probably doesn't make sense, right? What does "level" mean on a Transaction.

Yeah it might be one of those fields that have no place on transaction. It would be really nice to have some "official" list.

bruno-garcia · 2020-12-16T02:55:07Z

src/Sentry/SentryClient.cs

+            {
+                if (Random.NextDouble() > _options.TraceSampleRate)
+                {
+                    _options.DiagnosticLogger?.LogDebug("Transaction sampled.");


Suggested change

_options.DiagnosticLogger?.LogDebug("Transaction sampled.");

_options.DiagnosticLogger?.LogDebug("Transaction dropped due to random sampling.");

bruno-garcia · 2020-12-16T02:57:46Z

test/Sentry.Tests/SentryClientTests.cs

+        }
+
+        [Fact]
+        public void CaptureTransaction_InvalidTransaction_Ignored()


Why is it ignored?

Maybe: CaptureTransaction_NoName_Ignored

CaptureTransaction_NoOperation_Ignored

and one case per each?

bruno-garcia · 2020-12-16T02:59:08Z

@aleksihakli thanks for helping out with reviewing!

aleksihakli · 2020-12-16T11:10:07Z

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 · 2021-01-12T16:47:44Z

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 · 2021-01-12T20:41:01Z

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 👍

wip

f31adac

Tyrrrz added Feature New feature or request Sentry labels Nov 30, 2020

wip

79d2397

bruno-garcia reviewed Nov 30, 2020

View reviewed changes

lucas-zimerman reviewed Nov 30, 2020

View reviewed changes

src/Sentry/Protocol/ISpan.cs Outdated Show resolved Hide resolved

wip

d7c9974

Tyrrrz added 2 commits December 1, 2020 21:19

wip

51783e8

wip

c99635d

bruno-garcia reviewed Dec 14, 2020

View reviewed changes

src/Sentry.AspNetCore/ScopeExtensions.cs Show resolved Hide resolved

src/Sentry.AspNetCore/SentryMiddleware.cs Show resolved Hide resolved

Tyrrrz added 12 commits December 14, 2020 18:54

wip

134d0ec

wip

6b92788

wip

b6f3ee2

wip

0e91066

wip

ea06cfb

wip

2cb49ef

wip

e908cee

wip

e8aa3cc

wip

71c87ca

wip

0c512b1

wip

fbd6f17

wip

13b88db

Tyrrrz marked this pull request as ready for review December 14, 2020 21:12

wip

eb37814

aleksihakli approved these changes Dec 15, 2020

View reviewed changes

wip

61085a4

bruno-garcia approved these changes Dec 16, 2020

View reviewed changes

bruno-garcia merged commit 3bc6c66 into main Dec 16, 2020

bruno-garcia deleted the performance branch December 16, 2020 03:02

Tyrrrz mentioned this pull request Dec 16, 2020

Fix transaction review comments #657

Merged

bruno-garcia mentioned this pull request Jan 4, 2021

Performance Monitoring manual API #715

Closed

	_options.DiagnosticLogger?.LogDebug("Transaction sampled.");
	_options.DiagnosticLogger?.LogDebug("Transaction dropped due to random sampling.");

Support performance #633

Support performance #633

Conversation

Tyrrrz commented Nov 30, 2020

Tyrrrz commented Nov 30, 2020

Tyrrrz commented Nov 30, 2020

Tyrrrz commented Nov 30, 2020 • edited Loading

lucas-zimerman commented Nov 30, 2020

lucas-zimerman commented Nov 30, 2020

lucas-zimerman commented Nov 30, 2020

Tyrrrz commented Nov 30, 2020

lucas-zimerman commented Nov 30, 2020 • edited Loading

lucas-zimerman commented Nov 30, 2020

bruno-garcia 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

Tyrrrz Dec 1, 2020 • 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

Tyrrrz commented Nov 30, 2020

bruno-garcia commented Nov 30, 2020

Tyrrrz commented Nov 30, 2020

bruno-garcia commented Nov 30, 2020

Tyrrrz commented Dec 1, 2020 • edited Loading

Tyrrrz commented Dec 1, 2020

lucas-zimerman commented Dec 1, 2020

bruno-garcia commented Dec 1, 2020 • edited Loading

bruno-garcia commented Dec 1, 2020

Tyrrrz commented Dec 1, 2020

Tyrrrz commented Dec 1, 2020 • edited Loading

bruno-garcia commented Dec 1, 2020

codecov-io commented Dec 14, 2020 • edited Loading

Codecov Report

Tyrrrz commented Dec 14, 2020

aleksihakli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyrrrz commented Dec 15, 2020

bruno-garcia 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

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

bruno-garcia commented Dec 16, 2020

aleksihakli commented Dec 16, 2020

aleksihakli commented Jan 12, 2021 • edited Loading

bruno-garcia commented Jan 12, 2021 • edited Loading

Tyrrrz commented Nov 30, 2020 •

edited

Loading

lucas-zimerman commented Nov 30, 2020 •

edited

Loading

Tyrrrz Dec 1, 2020 •

edited

Loading

Tyrrrz commented Dec 1, 2020 •

edited

Loading

bruno-garcia commented Dec 1, 2020 •

edited

Loading

Tyrrrz commented Dec 1, 2020 •

edited

Loading

codecov-io commented Dec 14, 2020 •

edited

Loading

aleksihakli commented Jan 12, 2021 •

edited

Loading

bruno-garcia commented Jan 12, 2021 •

edited

Loading