Implement DroppedSpanStats #1511

gregkalapos · 2021-10-06T18:28:08Z

According to the spec this PR adds a new field to Transaction which contains aggregated data about dropped spans.

We had 1 optimization prior to this PR: we did not keep records of destination and resource for dropped spans, which we won't be able to do once we collect DroppenSpansStats.

apmmachine · 2021-10-06T19:20:43Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-11-09T16:47:26.726+0000
Duration: 89 min 13 sec
Commit: dc5dd49

Test stats 🧪

Test	Results
Failed	0
Passed	20239
Skipped	134
Total	20373

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run benchmark tests : Run the benchmark test.

russcam

I had a first pass and left some comments

russcam · 2021-10-11T04:31:22Z

src/Elastic.Apm/Model/Transaction.cs

+			if (_droppedSpanStatsMap.ContainsKey((destinationServiceResource, outcome)))
+			{
+				_droppedSpanStatsMap[(destinationServiceResource, outcome)].DurationCount++;
+				_droppedSpanStatsMap[(destinationServiceResource, outcome)].DurationSumUs += duration;
+			}
+			else
+			{
+				_droppedSpanStatsMap.Add((destinationServiceResource, outcome),
+					new DroppedSpanStats(destinationServiceResource, outcome, duration));
+			}
+		}


.TryGetValue(...) would likely perform better

Ok, changed it.

russcam · 2021-10-11T04:46:19Z

src/Elastic.Apm/Model/Transaction.cs

 		/// </summary>
-		[JsonIgnore]
-		public IConfiguration Configuration { get; }
+		private Dictionary<(string, Outcome), DroppedSpanStats> _droppedSpanStatsMap;


An internal struct to represent (string, Outcome) might be preferable, and be a move towards resolving some of the issues encountered with System.ValueTuple and .NET Framework from time to time e.g.

https://discuss.elastic.co/t/system-valuetuple-and-netframework-4-7-2/281768/2

https://discuss.elastic.co/t/elastic-apm-and-netframework-4-8/262896

Oh - yeah, I introduced a struct for this.

russcam · 2021-10-11T04:47:41Z

src/Elastic.Apm/Model/DbSpanCommon.cs

+				capturedSpan.Context.Db = new Database
 				{
-					capturedSpan.Context.Db = new Database
-					{
-						Statement = GetDbSpanName(dbCommand),
-						Instance = dbCommand.Connection.Database,
-						Type = Database.TypeSql
-					};
-
-					capturedSpan.Context.Destination = GetDestination(dbCommand.Connection?.ConnectionString, defaultPort);
-				}
+					Statement = GetDbSpanName(dbCommand), Instance = dbCommand.Connection.Database, Type = Database.TypeSql
+				};
+
+				capturedSpan.Context.Destination = GetDestination(dbCommand.Connection?.ConnectionString, defaultPort);


Could these now in theory be captured when starting the span?

Yes, I think actually this could have been in StartSpan(...) prior to this change as well. Although I don't think it changes much.

gregkalapos · 2021-10-18T20:57:45Z

@russcam this is ready for another round.

russcam

LGTM. I left some suggestions for small changes.

One thing that looks missing from this PR is documentation for dropped span stats. Should that be added?

src/Elastic.Apm/Model/Transaction.cs

Introduce DroppedSpanStatsKey

gregkalapos · 2021-11-03T17:56:01Z

One thing that looks missing from this PR is documentation for dropped span stats. Should that be added?

I don't think we need any extra docs in this repo. This is following the spec which documents this.

gregkalapos · 2021-11-03T20:11:55Z

Transaction_And_Spans_Captured_When_Large_Request fails. It's green locally.

gregkalapos · 2021-11-08T18:51:24Z

Introduced a field to store Context.Destination.Service.Resource on the top level on Span. With that, we can avoid allocating the whole Context object when a span is dropped.

Prior to the commit (benchmark is included - it's 1 transaction with 10 spans, 9 of those are dropped):

| Method |     Mean |    Error |   StdDev |   Gen 0 | Gen 1 | Gen 2 | Allocated |
|------- |---------:|---------:|---------:|--------:|------:|------:|----------:|
|      V | 12.07 ms | 0.231 ms | 0.204 ms | 15.6250 |     - |     - | 173.13 KB |

After the commit:

| Method |     Mean |    Error |   StdDev |   Gen 0 | Gen 1 | Gen 2 | Allocated |
|------- |---------:|---------:|---------:|--------:|------:|------:|----------:|
|      V | 12.76 ms | 2.177 ms | 6.175 ms | 15.6250 |     - |     - | 126.52 KB |

gregkalapos · 2021-11-08T21:47:57Z

Transaction_And_Spans_Captured_When_Large_Request still fails.

Another benchmark, 1000spans, 1sampled, the rest isn't.

This PR:

|        Method |     Mean |     Error |    StdDev |    Gen 0 |  Gen 1 | Gen 2 | Allocated |
|-------------- |---------:|----------:|----------:|---------:|-------:|------:|----------:|
| Test1000Spans | 3.260 ms | 0.0790 ms | 0.0660 ms | 941.4063 | 7.8125 |     - |   5.64 MB |

master:

|        Method |     Mean |     Error |    StdDev |    Gen 0 |  Gen 1 | Gen 2 | Allocated |
|-------------- |---------:|----------:|----------:|---------:|-------:|------:|----------:|
| Test1000Spans | 3.201 ms | 0.0390 ms | 0.0365 ms | 941.4063 | 7.8125 |     - |   5.64 MB |

So as it seems we don't really add overhead to spans that are not sampled.

gregkalapos · 2021-11-08T23:15:11Z

Some more findings:

This PR 1K spans, 0,5K sampled

|        Method |     Mean |     Error |    StdDev |    Gen 0 |    Gen 1 | Gen 2 | Allocated |
|-------------- |---------:|----------:|----------:|---------:|---------:|------:|----------:|
| Test1000Spans | 5.822 ms | 0.1163 ms | 0.1845 ms | 968.7500 | 250.0000 |     - |    5.8 MB |

master:

|        Method |     Mean |     Error |    StdDev |    Gen 0 |    Gen 1 | Gen 2 | Allocated |
|-------------- |---------:|----------:|----------:|---------:|---------:|------:|----------:|
| Test1000Spans | 6.035 ms | 0.1282 ms | 0.2070 ms | 960.9375 | 242.1875 |     - |   5.79 MB |

This is all .NET Core - I don't really see any sign of a huge overhead added by this PR.

The reason Transaction_And_Spans_Captured_When_Large_Request fails is clearly the number of spans it generates - it takes 15+sec already locally on me without this PR.

Now, the Transaction_And_Spans_Captured_When_Large_Request should assert that large requests are captured properly and the 1K span generated in the test is just a side effect. I changed the test to generate less spans but kept the large request size.

…-agent-dotnet into DroppedSpanStats

gregkalapos self-assigned this Oct 6, 2021

github-actions bot added the agent-dotnet label Oct 6, 2021

russcam reviewed Oct 11, 2021

View reviewed changes

gregkalapos requested a review from russcam October 18, 2021 20:57

russcam approved these changes Nov 2, 2021

View reviewed changes

src/Elastic.Apm/Model/Transaction.cs Outdated Show resolved Hide resolved

src/Elastic.Apm/Model/Transaction.cs Outdated Show resolved Hide resolved

src/Elastic.Apm/Model/Transaction.cs Show resolved Hide resolved

gregkalapos added 4 commits November 3, 2021 18:53

Implement DroppedSpanStats

3103d7c

Dropped spans: calculate destination for dropped spans stats

292fd58

Update Transaction.cs

28f522e

Introduce DroppedSpanStatsKey

Update Transaction.cs

471fc12

Update DroppedSpansStatsTests.cs

093e646

russcam mentioned this pull request Nov 4, 2021

Performance improvement: Agent should not auto capture spans that will be discarded #474

Closed

Do not create Span.Context for dropped spans

176fa5b

gregkalapos added 3 commits November 8, 2021 20:27

Update ExitSpanTests.cs

ed3cd95

Update Span.cs

2b6d0c5

Remove obsolete fields

bf4e9cb

gregkalapos added 2 commits November 8, 2021 23:03

Update DroppedSpansStats.cs

d7b2626

Generate less spans in Transaction_And_Spans_Captured_When_Large_Request

23db266

gregkalapos added 5 commits November 9, 2021 00:15

Merge branch 'master' into DroppedSpanStats

4ffe18c

Update HttpContextCurrentExecutionSegmentsContainerTests.cs

c0d5ec3

Merge branch 'DroppedSpanStats' of https://github.com/gregkalapos/apm…

ea87e94

…-agent-dotnet into DroppedSpanStats

Merge branch 'master' into DroppedSpanStats

df666bb

Adapt test - remove unused fields

dc5dd49

gregkalapos merged commit 4c4d2c0 into elastic:master Nov 9, 2021

gregkalapos deleted the DroppedSpanStats branch November 9, 2021 18:35

This was referenced Nov 9, 2021

Collecting statistics about dropped spans elastic/apm#497

Open

[META 497] Collecting statistics about dropped spans #1476

Closed

russcam added v1.12.0 enhancement New feature or request labels Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement DroppedSpanStats #1511

Implement DroppedSpanStats #1511

gregkalapos commented Oct 6, 2021

apmmachine commented Oct 6, 2021 •

edited

Loading

Build stats

Test stats 🧪

russcam left a comment

russcam Oct 11, 2021

gregkalapos Oct 18, 2021

russcam Oct 11, 2021

gregkalapos Oct 18, 2021

russcam Oct 11, 2021

gregkalapos Oct 18, 2021

gregkalapos commented Oct 18, 2021

russcam left a comment

gregkalapos commented Nov 3, 2021

gregkalapos commented Nov 3, 2021

gregkalapos commented Nov 8, 2021 •

edited

Loading

gregkalapos commented Nov 8, 2021

gregkalapos commented Nov 8, 2021 •

edited

Loading

Implement DroppedSpanStats #1511

Implement DroppedSpanStats #1511

Conversation

gregkalapos commented Oct 6, 2021

apmmachine commented Oct 6, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

russcam left a comment

Choose a reason for hiding this comment

russcam Oct 11, 2021

Choose a reason for hiding this comment

gregkalapos Oct 18, 2021

Choose a reason for hiding this comment

russcam Oct 11, 2021

Choose a reason for hiding this comment

gregkalapos Oct 18, 2021

Choose a reason for hiding this comment

russcam Oct 11, 2021

Choose a reason for hiding this comment

gregkalapos Oct 18, 2021

Choose a reason for hiding this comment

gregkalapos commented Oct 18, 2021

russcam left a comment

Choose a reason for hiding this comment

gregkalapos commented Nov 3, 2021

gregkalapos commented Nov 3, 2021

gregkalapos commented Nov 8, 2021 • edited Loading

gregkalapos commented Nov 8, 2021

gregkalapos commented Nov 8, 2021 • edited Loading

apmmachine commented Oct 6, 2021 •

edited

Loading

gregkalapos commented Nov 8, 2021 •

edited

Loading

gregkalapos commented Nov 8, 2021 •

edited

Loading