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 EnumerateTagValues Activity Extension #1236

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Sep 4, 2020

Changes

The ask on #1221 was for a way to find multiple tags. What I thought would be more useful is an extension for performing the allocation free foreach that users can call to do whatever they need with tags.

Usage

  1. Define your state struct and implement the ForEach body. This must be a struct:

         private struct Enumerator : IActivityTagEnumerator
         {
             public int Count { get; private set; }
    
             public bool ForEach(KeyValuePair<string, object> item)
             {
                 this.Count++;
                 return true;
             }
         }
  2. Call the extension, pass the state:

             Enumerator state = default;
    
             Activity.EnumerateTagValues(ref state);
  3. Use the state to retrieve any results you might need:

         public static int CountTags(Activity activity)
         {
             Enumerator state = default;
    
             activity.EnumerateTagValues(ref state);
    
             return state.Count;
         }

Benchmarks

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
EnumerateNonemptyTagObjects 16,142.977 ns 308.3928 ns 629.9648 ns - - - 40 B
EnumerateTagValuesNonemptyTagObjects 9,183.533 ns 136.4799 ns 120.9859 ns - - - -
  • CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@CodeBlanch CodeBlanch requested a review from a team September 4, 2020 17:13
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #1236 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1236      +/-   ##
==========================================
+ Coverage   78.89%   78.98%   +0.08%     
==========================================
  Files         219      219              
  Lines        6331     6347      +16     
==========================================
+ Hits         4995     5013      +18     
+ Misses       1336     1334       -2     
Impacted Files Coverage Δ
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 98.18% <100.00%> (+0.74%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️
...us/Implementation/PrometheusExporterEventSource.cs 72.72% <0.00%> (+9.09%) ⬆️

{
return null;
}
Debug.Assert(activity != null, "Activity should not be null");
Copy link
Member

Choose a reason for hiding this comment

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

Minor: consider the stock Assert.IsNotNull.

Copy link
Member Author

Choose a reason for hiding this comment

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

You want me to add Xunit to API? 🤣 I don't think there's a similar method on System.Diagnostics.Debug, but there should be!

Copy link
Member

Choose a reason for hiding this comment

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

Not familiar with .NET, I thought Assert.IsNotNull is a common thing and am surprised that it is not (so it looks like a compile time trick).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry didn't mean to poke fun 😄 Ya it's common to unit test frameworks but I don't think there's anything in the BCL.

Copy link
Member

Choose a reason for hiding this comment

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

I took a look at the doc and it makes me wonder if we should use it or throw normal exception.

It seems in C++ the general trend is to avoid https://en.cppreference.com/w/c/error/assert from a library.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can switch them to regular null checks if you would like. My thinking with the Debug.Assert was... user will have to work really hard to call these extensions with a null Activity. You have to go out of your way and do OpenTelemetry.Trace.ActivityExtensions.GetTagValue(null, "myTag"). If you are set on shooting yourself in the foot, who are we to stop you? 🤣 But for the 99.99% of callers who are doing activity.GetTagValue("myTag") it really will never be null, so I was trying to save the perf of doing an unnecessary check.

Copy link
Member

Choose a reason for hiding this comment

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

I was trying to get your perspective and see if I need to change this

.

Based on the discussion, I think we should remove the null check for extension methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is kind of a risk/reward thing. That particular extension would only be called a handful of times (?) so it's probably fine to leave the check in there. If we don't have the check, static analysis tools like FxCop get upset so then we end up with suppressions which are kind of a maintainability headache. IMO we should only do this on hot-path stuff where we know there is a compelling benefit to justify the suppression. Assuming we get to turning FxCop on 😄 Poor @eddynaka has been trying to do that for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

its getting harder and harder :(

Copy link
Member

Choose a reason for hiding this comment

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

Coming into the middle of this conversation, though reminds me of System.Diagnostics.Contracts.

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.

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.

5 participants