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 Activity.EnumerateLinks extension #1314

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Sep 29, 2020

Added an extension method for enumerating Activity Links without an allocation. Updated Jaeger to use it.

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
EnumerateNonemptyActivityLinks 94.488 ns 0.6160 ns 0.5762 ns 0.0076 - - 64 B
EnumerateLinksNonemptyActivityLinks 48.164 ns 0.4717 ns 0.4181 ns - - - -

TODOs:

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

@CodeBlanch CodeBlanch requested a review from a team September 29, 2020 01:11
/// </summary>
public interface IActivityTagEnumerator
/// <typeparam name="T">Enumerated item type.</typeparam>
public interface IActivityEnumerator<T>
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 switched IActivityTagEnumerator to IActivityEnumerator<T> so I could use it for tags, links, and events (up next). Breaking change, which is a con, but pro is with this change we don't need a bunch of interfaces that are basically the same, just different item types.

@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #1314 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1314      +/-   ##
==========================================
+ Coverage   78.51%   78.63%   +0.11%     
==========================================
  Files         219      219              
  Lines        6243     6253      +10     
==========================================
+ Hits         4902     4917      +15     
+ Misses       1341     1336       -5     
Impacted Files Coverage Δ
...plementation/ZipkinActivityConversionExtensions.cs 92.07% <ø> (ø)
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 98.59% <100.00%> (+0.40%) ⬆️
....Jaeger/Implementation/JaegerActivityExtensions.cs 96.20% <100.00%> (+0.47%) ⬆️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 100.00% <0.00%> (+2.85%) ⬆️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 80.95% <0.00%> (+3.17%) ⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

@CodeBlanch
Copy link
Member Author

@eddynaka @cijothomas Just updated branch and I'm getting a strange, seemingly unrelated, test failure:

X OpenTelemetry.Tests.EventSourceTest.EventSourceTest_OpenTelemetrySdkEventSource [25ms]
  Error Message:
   System.Exception : Method 'OpenTelemetrySdkEventSource.MetricObserverCallbackError' is implemented incorrectly.

Any idea what might be causing that?

@eddynaka
Copy link
Contributor

eddynaka commented Oct 2, 2020

@eddynaka @cijothomas Just updated branch and I'm getting a strange, seemingly unrelated, test failure:

X OpenTelemetry.Tests.EventSourceTest.EventSourceTest_OpenTelemetrySdkEventSource [25ms]
  Error Message:
   System.Exception : Method 'OpenTelemetrySdkEventSource.MetricObserverCallbackError' is implemented incorrectly.

Any idea what might be causing that?

It's an order issue. I couldn't find a way to trigger. but sometimes happens.

@CodeBlanch CodeBlanch merged commit 8e7ba79 into open-telemetry:master Oct 2, 2020
@CodeBlanch CodeBlanch deleted the activity-enumeratelinksextension branch October 2, 2020 18:16
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.

2 participants