From 959f4438cb9f9fe499d76e1dab2fd11dcfa22012 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 29 Jan 2021 16:47:40 -0800 Subject: [PATCH] JaegerExporter batching changes (#1732) * Removed the logic in JaegerExporter for generating batches by process. * CHANGELOG update. Co-authored-by: Cijo Thomas --- .../CHANGELOG.md | 8 ++- .../Implementation/Batch.cs | 6 -- .../JaegerExporter.cs | 65 ++++------------- .../JaegerExporterTests.cs | 71 +++---------------- 4 files changed, 28 insertions(+), 122 deletions(-) diff --git a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md index 8a535c1ed20..d09a259e4b2 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md +++ b/src/OpenTelemetry.Exporter.Jaeger/CHANGELOG.md @@ -5,8 +5,8 @@ * Changed `JaegerExporter` class and constructor from internal to public. ([#1612](https://github.com/open-telemetry/opentelemetry-dotnet/issues/1612)) -* In `JaegerExporterOptions`: Exporter options now include a switch for - Batch vs Simple exporter, and settings for batch exporting properties. +* In `JaegerExporterOptions`: Exporter options now include a switch for Batch vs + Simple exporter, and settings for batch exporting properties. * Jaeger will now set the `error` tag when `otel.status_code` is set to `ERROR`. ([#1579](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1579) & @@ -20,6 +20,10 @@ instead of `message`. ([#1609](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1609)) +* `JaegerExporter` batch format has changed to be compliant with the spec. This + may impact the way spans are displayed in Jaeger UI. + ([#1732](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1732)) + ## 1.0.0-rc1.1 Released 2020-Nov-17 diff --git a/src/OpenTelemetry.Exporter.Jaeger/Implementation/Batch.cs b/src/OpenTelemetry.Exporter.Jaeger/Implementation/Batch.cs index a9cff7bb03d..36965771c09 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/Implementation/Batch.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/Implementation/Batch.cs @@ -105,11 +105,5 @@ internal void Clear() { PooledList.Clear(ref this.spanMessages); } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void Return() - { - this.spanMessages.Return(); - } } } diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs index 344139600f1..3ac56273dab 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs +++ b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporter.cs @@ -36,7 +36,6 @@ public class JaegerExporter : BaseExporter private readonly JaegerThriftClient thriftClient; private readonly InMemoryTransport memoryTransport; private readonly TProtocol memoryProtocol; - private Dictionary processCache; private int batchByteSize; private bool disposedValue; // To detect redundant dispose calls @@ -64,16 +63,16 @@ internal JaegerExporter(JaegerExporterOptions options, TTransport clientTranspor internal Process Process { get; set; } - internal Dictionary CurrentBatches { get; } = new Dictionary(); + internal Batch Batch { get; private set; } /// public override ExportResult Export(in Batch activityBatch) { try { - if (this.processCache == null) + if (this.Batch == null) { - this.SetResource(this.ParentProvider.GetResource()); + this.SetResourceAndInitializeBatch(this.ParentProvider.GetResource()); } foreach (var activity in activityBatch) @@ -81,7 +80,7 @@ public override ExportResult Export(in Batch activityBatch) this.AppendSpan(activity.ToJaegerSpan()); } - this.SendCurrentBatches(null); + this.SendCurrentBatch(); return ExportResult.Success; } @@ -93,7 +92,7 @@ public override ExportResult Export(in Batch activityBatch) } } - internal void SetResource(Resource resource) + internal void SetResourceAndInitializeBatch(Resource resource) { if (resource is null) { @@ -142,48 +141,25 @@ internal void SetResource(Resource resource) } this.Process.Message = this.BuildThriftMessage(this.Process).ToArray(); - this.processCache = new Dictionary - { - [this.Process.ServiceName] = this.Process, - }; + this.Batch = new Batch(this.Process); + this.batchByteSize = this.Process.Message.Length; } [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void AppendSpan(JaegerSpan jaegerSpan) { - var spanServiceName = jaegerSpan.PeerServiceName ?? this.Process.ServiceName; - - if (!this.processCache.TryGetValue(spanServiceName, out var spanProcess)) - { - spanProcess = new Process(spanServiceName, this.Process.Tags); - spanProcess.Message = this.BuildThriftMessage(spanProcess).ToArray(); - this.processCache.Add(spanServiceName, spanProcess); - } - var spanMessage = this.BuildThriftMessage(jaegerSpan); jaegerSpan.Return(); var spanTotalBytesNeeded = spanMessage.Count; - if (!this.CurrentBatches.TryGetValue(spanServiceName, out var spanBatch)) - { - spanBatch = new Batch(spanProcess); - this.CurrentBatches.Add(spanServiceName, spanBatch); - - spanTotalBytesNeeded += spanProcess.Message.Length; - } - if (this.batchByteSize + spanTotalBytesNeeded >= this.maxPayloadSizeInBytes) { - this.SendCurrentBatches(spanBatch); - - // Flushing effectively erases the spanBatch we were working on, so we have to rebuild it. - spanTotalBytesNeeded = spanMessage.Count + spanProcess.Message.Length; - this.CurrentBatches.Add(spanServiceName, spanBatch); + this.SendCurrentBatch(); } - spanBatch.Add(spanMessage); + this.Batch.Add(spanMessage); this.batchByteSize += spanTotalBytesNeeded; } @@ -206,30 +182,17 @@ protected override void Dispose(bool disposing) base.Dispose(disposing); } - private void SendCurrentBatches(Batch workingBatch) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void SendCurrentBatch() { try { - foreach (var batchKvp in this.CurrentBatches) - { - var batch = batchKvp.Value; - - this.thriftClient.SendBatch(batch); - - if (batch != workingBatch) - { - batch.Return(); - } - else - { - batch.Clear(); - } - } + this.thriftClient.SendBatch(this.Batch); } finally { - this.CurrentBatches.Clear(); - this.batchByteSize = 0; + this.Batch.Clear(); + this.batchByteSize = this.Process.Message.Length; this.memoryTransport.Reset(); } } diff --git a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterTests.cs b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterTests.cs index fbc56fed39d..9fbb44f3510 100644 --- a/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterTests.cs +++ b/test/OpenTelemetry.Exporter.Jaeger.Tests/JaegerExporterTests.cs @@ -52,15 +52,15 @@ public void JaegerTraceExporter_SetResource_UpdatesServiceName() process.ServiceName = "TestService"; - jaegerTraceExporter.SetResource(Resource.Empty); + jaegerTraceExporter.SetResourceAndInitializeBatch(Resource.Empty); Assert.Equal("TestService", process.ServiceName); - jaegerTraceExporter.SetResource(ResourceBuilder.CreateEmpty().AddService("MyService").Build()); + jaegerTraceExporter.SetResourceAndInitializeBatch(ResourceBuilder.CreateEmpty().AddService("MyService").Build()); Assert.Equal("MyService", process.ServiceName); - jaegerTraceExporter.SetResource(ResourceBuilder.CreateEmpty().AddService("MyService", "MyNamespace").Build()); + jaegerTraceExporter.SetResourceAndInitializeBatch(ResourceBuilder.CreateEmpty().AddService("MyService", "MyNamespace").Build()); Assert.Equal("MyNamespace.MyService", process.ServiceName); } @@ -71,7 +71,7 @@ public void JaegerTraceExporter_SetResource_CreatesTags() using var jaegerTraceExporter = new JaegerExporter(new JaegerExporterOptions()); var process = jaegerTraceExporter.Process; - jaegerTraceExporter.SetResource(ResourceBuilder.CreateEmpty().AddAttributes(new Dictionary + jaegerTraceExporter.SetResourceAndInitializeBatch(ResourceBuilder.CreateEmpty().AddAttributes(new Dictionary { ["Tag"] = "value", }).Build()); @@ -89,7 +89,7 @@ public void JaegerTraceExporter_SetResource_CombinesTags() process.Tags = new Dictionary { ["Tag1"] = new KeyValuePair("Tag1", "value1").ToJaegerTag() }; - jaegerTraceExporter.SetResource(ResourceBuilder.CreateEmpty().AddAttributes(new Dictionary + jaegerTraceExporter.SetResourceAndInitializeBatch(ResourceBuilder.CreateEmpty().AddAttributes(new Dictionary { ["Tag2"] = "value2", }).Build()); @@ -106,7 +106,7 @@ public void JaegerTraceExporter_SetResource_IgnoreServiceResources() using var jaegerTraceExporter = new JaegerExporter(new JaegerExporterOptions()); var process = jaegerTraceExporter.Process; - jaegerTraceExporter.SetResource(ResourceBuilder.CreateEmpty().AddAttributes(new Dictionary + jaegerTraceExporter.SetResourceAndInitializeBatch(ResourceBuilder.CreateEmpty().AddAttributes(new Dictionary { [ResourceSemanticConventions.AttributeServiceName] = "servicename", [ResourceSemanticConventions.AttributeServiceNamespace] = "servicenamespace", @@ -115,75 +115,20 @@ public void JaegerTraceExporter_SetResource_IgnoreServiceResources() Assert.Null(process.Tags); } - [Fact] - public void JaegerTraceExporter_BuildBatchesToTransmit_DefaultBatch() - { - // Arrange - using var jaegerExporter = new JaegerExporter(new JaegerExporterOptions()); - jaegerExporter.SetResource(Resource.Empty); - - // Act - jaegerExporter.AppendSpan(CreateTestJaegerSpan()); - jaegerExporter.AppendSpan(CreateTestJaegerSpan()); - jaegerExporter.AppendSpan(CreateTestJaegerSpan()); - - var batches = jaegerExporter.CurrentBatches.Values; - - // Assert - Assert.Single(batches); - Assert.Equal(DefaultServiceName, batches.First().Process.ServiceName); - Assert.Equal(3, batches.First().Count); - } - - [Fact] - public void JaegerTraceExporter_BuildBatchesToTransmit_MultipleBatches() - { - // Arrange - using var jaegerExporter = new JaegerExporter(new JaegerExporterOptions()); - jaegerExporter.SetResource(Resource.Empty); - - // Act - jaegerExporter.AppendSpan(CreateTestJaegerSpan()); - jaegerExporter.AppendSpan( - CreateTestJaegerSpan( - additionalAttributes: new Dictionary - { - ["peer.service"] = "MySQL", - })); - jaegerExporter.AppendSpan(CreateTestJaegerSpan()); - - var batches = jaegerExporter.CurrentBatches.Values; - - // Assert - Assert.Equal(2, batches.Count()); - - var primaryBatch = batches.Where(b => b.Process.ServiceName == DefaultServiceName); - Assert.Single(primaryBatch); - Assert.Equal(2, primaryBatch.First().Count); - - var mySQLBatch = batches.Where(b => b.Process.ServiceName == "MySQL"); - Assert.Single(mySQLBatch); - Assert.Equal(1, mySQLBatch.First().Count); - } - [Fact] public void JaegerTraceExporter_BuildBatchesToTransmit_FlushedBatch() { // Arrange using var jaegerExporter = new JaegerExporter(new JaegerExporterOptions { MaxPayloadSizeInBytes = 1500 }); - jaegerExporter.SetResource(Resource.Empty); + jaegerExporter.SetResourceAndInitializeBatch(Resource.Empty); // Act jaegerExporter.AppendSpan(CreateTestJaegerSpan()); jaegerExporter.AppendSpan(CreateTestJaegerSpan()); jaegerExporter.AppendSpan(CreateTestJaegerSpan()); - var batches = jaegerExporter.CurrentBatches.Values; - // Assert - Assert.Single(batches); - Assert.Equal(DefaultServiceName, batches.First().Process.ServiceName); - Assert.Equal(1, batches.First().Count); + Assert.Equal(1, jaegerExporter.Batch.Count); } internal static JaegerSpan CreateTestJaegerSpan(