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

Updated Jaeger & Zipkin exporters to check for default activity.ParentSpanId #1433

Merged
merged 3 commits into from
Nov 2, 2020

Conversation

CodeBlanch
Copy link
Member

Changes

OtlpExporter was slightly smarter than Zipkin and Jaeger in how it handled activity.ParentSpanId. This makes the logic the same which should be a slight performance boost for root spans.

@CodeBlanch CodeBlanch requested a review from a team October 31, 2020 17:46
@@ -55,5 +76,44 @@ protected void Application_End()
{
this.tracerProvider?.Dispose();
}

private class DebugStreamWriter : StreamWriter
Copy link
Member Author

Choose a reason for hiding this comment

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

The .NET Framework example is a web application so I added this to redirect console output to the VS debug window.

Copy link
Member

Choose a reason for hiding this comment

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

Might be a good addition to the ConsoleExporter.

Copy link
Member Author

Choose a reason for hiding this comment

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

@reyang You thinking something like adding an option to have ConsoleExporter switch to Debug.Write instead of Console.Write?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I see a general need for processes that don't have a "console", and I think having yet another "Debug stream exporter" might be an overkill.
In C++ we have "ostream exporter" which allows whatever output stream, and the default is STDOUT. And I think it is a good approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #1433 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1433      +/-   ##
==========================================
+ Coverage   81.83%   81.85%   +0.01%     
==========================================
  Files         227      227              
  Lines        6089     6089              
==========================================
+ Hits         4983     4984       +1     
+ Misses       1106     1105       -1     
Impacted Files Coverage Δ
....Jaeger/Implementation/JaegerActivityExtensions.cs 96.06% <100.00%> (+0.03%) ⬆️
...plementation/ZipkinActivityConversionExtensions.cs 98.66% <100.00%> (-0.02%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️

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.

@cijothomas cijothomas merged commit ddac284 into open-telemetry:master Nov 2, 2020
@CodeBlanch CodeBlanch deleted the exporter-parentid branch November 3, 2020 19:36
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