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 a Jaeger Activity Exporter #693

Merged

Conversation

pjanotti
Copy link
Contributor

Changes

Add a Jaeger Activity Exporter. The code uses the same patterns already in the Jaeger Exporter and adds a new exporter capable of transforming Activity instances into Jaeger spans.

Checklist

  • I ran Unit Tests locally.
  • Sample exporter against OTel Collector.

@@ -89,7 +89,7 @@ public async Task JaegerExporter_Batching()
{
for (int c = 0; c < this.NumberOfSpans; c++)
{
await jaegerUdpBatcher.AppendAsync(this.testSpan, CancellationToken.None).ConfigureAwait(false);
await jaegerUdpBatcher.AppendAsync(this.testSpan.ToJaegerSpan(), CancellationToken.None).ConfigureAwait(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prior to this PR ToJaegerSpan() was called inside AppendAsync so the amount of work in the benchmark it is still the same.

{
internal static class JaegerActivityExtensions
{
private static readonly Dictionary<string, int> PeerServiceKeyResolutionDictionary = new Dictionary<string, int>(StringComparer.OrdinalIgnoreCase)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, this is duplicated with the extensions for SDK span. Likely should move to a common location.

@CodeBlanch
Copy link
Member

LGTM

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Left couple of minor comments about code comments.

@cijothomas
Copy link
Member

Leaving unmerged for at least a day to give everyone a chance to review.

@cijothomas cijothomas merged commit cab77a9 into open-telemetry:master May 23, 2020
Illia-M added a commit to gr8-toolkit/opentelemetry-dotnet that referenced this pull request May 25, 2020
@cijothomas cijothomas mentioned this pull request May 27, 2020
32 tasks
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.

None yet

3 participants