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

Added GetTagValue Activity Extension #1221

Merged
merged 5 commits into from
Sep 3, 2020

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Sep 2, 2020

All the struct enumerators I worked with the .NET team to introduce are available in RC1 so now we can have some fun with the perf.

Changes

This adds a GetTagValue extension on Activity for finding a tag value by key very efficiently.

Benchmarks

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
EnumerateEmptyTagObjects 8.776 ns 0.1528 ns 0.1355 ns - - - -
LinqEmptyTagObjects 21.909 ns 0.4546 ns 0.4465 ns - - - -
GetTagValueEmptyTagObjects 1.826 ns 0.0335 ns 0.0297 ns - - - -
EnumerateNonemptyTagObjects 69.014 ns 1.3853 ns 1.2958 ns 0.0048 - - 40 B
LinqNonemptyTagObjects 89.760 ns 1.6389 ns 1.5331 ns 0.0048 - - 40 B
GetTagValueNonemptyTagObjects 49.510 ns 0.2616 ns 0.2042 ns - - - -

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@CodeBlanch CodeBlanch requested a review from a team September 2, 2020 05:39
@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #1221 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1221      +/-   ##
==========================================
+ Coverage   78.76%   78.81%   +0.04%     
==========================================
  Files         219      219              
  Lines        6311     6329      +18     
==========================================
+ Hits         4971     4988      +17     
- Misses       1340     1341       +1     
Impacted Files Coverage Δ
src/OpenTelemetry/Internal/EnumerationHelper.cs 91.42% <ø> (ø)
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 97.43% <100.00%> (+2.19%) ⬆️
...try.Instrumentation.GrpcNetClient/GrpcTagHelper.cs 100.00% <100.00%> (ø)
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️
...us/Implementation/PrometheusExporterEventSource.cs 72.72% <0.00%> (+9.09%) ⬆️

@eddynaka
Copy link
Contributor

eddynaka commented Sep 2, 2020

Don't know how hard is, but we could add another test using Expression.Lambda and compare the perf. The same that we did when we were trying to create the SetKind. What do you think?

/// <param name="tagName">Case-sensitive tag name to retrieve.</param>
/// <returns>Tag value or null if a match was not found.</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static object GetTagValue(this Activity activity, string tagName)
Copy link
Member

Choose a reason for hiding this comment

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

Curious - I wonder if there are many scenarios where people would try to get multiple tag values. If yes, a further improvement might be giving a GetTagValues overload (which takes an array/list of names and a ref to Span or something allocated on the stack).

Copy link
Member Author

Choose a reason for hiding this comment

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

There's actually a method a few lines up doing two lookups! 1 for status code, 1 for status description. I'll take a stab at this later. If the PR gets merged I'll do as a follow-up.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM!

@CodeBlanch
Copy link
Member Author

Update: I found the spots using Linq in the main solution and updated them to use the extension. So it is now providing immediate value. I was looking for TagObjects, they were using Tags. 🙄

@CodeBlanch
Copy link
Member Author

CodeBlanch commented Sep 2, 2020

@eddynaka

Don't know how hard is, but we could add another test using Expression.Lambda and compare the perf. The same that we did when we were trying to create the SetKind. What do you think?

I tried, and it is really hard! The one we did for setting kind on Activity, we knew both types (Activity & ActivityKind). The enumeration stuff is a whole different ballgame because you don't know the concrete type or the enumerator type it uses. All you really know is it is an IEnumerable<T>. In the case of TagObjects, it uses an internal linked list thing that is not public. The enumerator type is internal too.

I'm not convinced it is impossible to do using expression syntax, it's just going to take some time to figure it out. If you have any ideas please share! Maybe on Gitter so we don't spam this?

@cijothomas cijothomas merged commit 623ad06 into open-telemetry:master Sep 3, 2020
@CodeBlanch CodeBlanch deleted the activity-gettagvalue branch September 4, 2020 05:03
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.

4 participants