Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add benchmark for DiagnosticSourceSubscriber #1391

Merged

Conversation

alanwest
Copy link
Member

Apologies for delay. Here are some benchmark results of DiagnosticSource per recent conversations in SIG meetings.

#1347 raised the question of the impact of multiple DiagnosticSource subscribers for a single source. Currently, #1347 sets up an additional DiagnosticSource subscriber to capture HTTP response time metrics for ASP.NET Core. Another approach would be to refactor things and capture these metrics with the existing subscriber used for traces.

SubscriberCount UseIsEnabledFilter Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
1 False 82.34 ns 1.635 ns 1.529 ns 0.0012 - - 24 B
1 True 79.00 ns 1.438 ns 1.345 ns 0.0012 - - 24 B
2 False 143.92 ns 2.064 ns 1.931 ns 0.0012 - - 24 B
2 True 144.86 ns 2.822 ns 2.502 ns 0.0012 - - 24 B

There does appear to be an increase in computation time when invoking multiple subscribers. It was mentioned in a SIG meeting that using an event filter may also have a significant impact, however, these results do not seem to suggest that (perhaps if the filter were less trivial).

Questions

  1. Do people think the increase in computation time warrants solving capturing both metrics and traces with a single DiagnosticSource subscriber? I have some thoughts for how this could be done, though it will require some consideration of how to independently configure instrumentation to gather only metric data, only trace data, or both.
  2. If yes, how would people feel about moving forward with Capture http.server.duration metric for ASP.NET Core instrumentation #1347 as is with a separate subscriber and then refactor in a separate PR? I still have a little more work independent of the questions posed here to get Capture http.server.duration metric for ASP.NET Core instrumentation #1347 wrapped up.

@alanwest alanwest requested a review from a team October 23, 2020 21:49
@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #1391 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1391      +/-   ##
==========================================
- Coverage   81.34%   81.33%   -0.02%     
==========================================
  Files         226      226              
  Lines        6090     6090              
==========================================
- Hits         4954     4953       -1     
- Misses       1136     1137       +1     
Impacted Files Coverage Δ
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 79.41% <0.00%> (-2.95%) ⬇️

@eddynaka
Copy link
Contributor

If u try with 5 or 10, the time will increase proportionally?

@alanwest
Copy link
Member Author

If u try with 5 or 10, the time will increase proportionally?

Good question. Yes, it does appear to grow proportionally:

SubscriberCount UseIsEnabledFilter Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
1 False 77.99 ns 1.318 ns 1.233 ns 0.0012 - - 24 B
1 True 79.63 ns 1.625 ns 1.871 ns 0.0012 - - 24 B
2 False 142.42 ns 2.846 ns 3.799 ns 0.0012 - - 24 B
2 True 139.01 ns 1.544 ns 1.289 ns 0.0012 - - 24 B
5 False 344.90 ns 6.889 ns 5.752 ns 0.0010 - - 24 B
5 True 354.82 ns 6.882 ns 6.437 ns 0.0010 - - 24 B
10 False 672.19 ns 10.926 ns 9.685 ns 0.0010 - - 24 B
10 True 667.73 ns 10.416 ns 9.743 ns 0.0010 - - 24 B

@eddynaka
Copy link
Contributor

If u try with 5 or 10, the time will increase proportionally?

Good question. Yes, it does appear to grow proportionally:

SubscriberCount UseIsEnabledFilter Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
1 False 77.99 ns 1.318 ns 1.233 ns 0.0012 - - 24 B
1 True 79.63 ns 1.625 ns 1.871 ns 0.0012 - - 24 B
2 False 142.42 ns 2.846 ns 3.799 ns 0.0012 - - 24 B
2 True 139.01 ns 1.544 ns 1.289 ns 0.0012 - - 24 B
5 False 344.90 ns 6.889 ns 5.752 ns 0.0010 - - 24 B
5 True 354.82 ns 6.882 ns 6.437 ns 0.0010 - - 24 B
10 False 672.19 ns 10.926 ns 9.685 ns 0.0010 - - 24 B
10 True 667.73 ns 10.416 ns 9.743 ns 0.0010 - - 24 B

Thanks for adding that. We can see that we shouldn't add multiple since it will increase in a 60-70ns * subscriber!

this.UseIsEnabledFilter ? this.isEnabledFilter : null);

this.subscribers.Add(subscriber);
DiagnosticListener.AllListeners.Subscribe(subscriber);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use our own wrappers to make this consistent? subscriber.Subscribe() should do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

@cijothomas
Copy link
Member

cijothomas commented Oct 26, 2020

I think its okay to have multiple subscribers rather than refactor the trace/metric to share common subscriber. The benefit is small perf gain, but we need to do complex refactoring to ensure disabling Trace has no impact on Metric and vice versa etc.

Once .NET 6 ships with new Metric API, we can try to get the common .net libraries (asp.net core, httpclient) to emit metrics directly using it, which avoids the need of these instrumentations. Most .NET libraries are instrumented currently with EventCounter which current don't support dimensions. Hopefully, the new metric API will be leverage by the libraries to emit the metrics along with all the dimensions.

@cijothomas cijothomas merged commit a4d5a01 into open-telemetry:master Oct 26, 2020
@alanwest alanwest deleted the alanwest/benchmark-diagnosticsource branch November 4, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants