From d4b3cdec73df4ee08227f6d5899b1c63e49fa274 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Thu, 15 Aug 2024 14:11:01 +0200 Subject: [PATCH 1/3] Adding filters should not force initialization of Agent. Instead if the agent is not yet setup we simply buffer the filters until the agent gets constructed. This allows you to add filters before initializing the static agent. This manifested itself in the hosted service integrations (.NET Core) where the agent get's constructed slightly delayed. And calling ``` Agent.AddFilter((ISpan span) => { return span; }); ``` Would win the race for `Agent.Instance` over the ASP.NET core integrations. --- src/Elastic.Apm/Agent.cs | 59 +++++++++++++++---- src/Elastic.Apm/Report/IPayloadSender.cs | 9 +++ src/Elastic.Apm/Report/PayloadSenderV2.cs | 35 +++++++++-- .../MockPayloadSender.cs | 37 +++++++++--- .../NoopPayloadSender.cs | 11 +++- test/Elastic.Apm.Tests/FilterTests.cs | 41 +++++++++---- 6 files changed, 158 insertions(+), 34 deletions(-) diff --git a/src/Elastic.Apm/Agent.cs b/src/Elastic.Apm/Agent.cs index b5630b16d..da85e8a5a 100644 --- a/src/Elastic.Apm/Agent.cs +++ b/src/Elastic.Apm/Agent.cs @@ -39,7 +39,6 @@ public interface IApmAgent : IApmAgentComponents internal class ApmAgent : IApmAgent, IDisposable { internal readonly CompositeDisposable Disposables = new(); - internal ApmAgent(AgentComponents agentComponents) => Components = agentComponents ?? new AgentComponents(); internal ICentralConfigurationFetcher CentralConfigurationFetcher => Components.CentralConfigurationFetcher; @@ -72,16 +71,30 @@ public static class Agent lock (InitializationLock) { var agent = new ApmAgent(Components); - agent.Logger?.Trace() ?.Log("Initialization - Agent instance initialized. Callstack: {callstack}", new StackTrace().ToString()); + if (agent.Components.PayloadSender is not IPayloadSenderWithFilters sender) return agent; + + ErrorFilters.ForEach(f => sender.AddFilter(f)); + TransactionFilters.ForEach(f => sender.AddFilter(f)); + SpanFilters.ForEach(f => sender.AddFilter(f)); + agent.Logger?.Trace() + ?.Log(@"Initialization - Added filters to agent (errors:{{ErrorFilters}}, transactions:{TransactionFilters} spans:{SpanFilters}", + ErrorFilters.Count, TransactionFilters.Count, SpanFilters.Count); + return agent; } }); private static readonly object InitializationLock = new object(); + private static readonly List> ErrorFilters = []; + + private static readonly List> SpanFilters = []; + + private static readonly List> TransactionFilters = []; + internal static AgentComponents Components { get; private set; } public static IConfigurationReader Config => Instance.Configuration; @@ -114,7 +127,16 @@ public static class Agent /// true if the filter was added successfully, false otherwise. In case the method /// returns false the filter won't be called. /// - public static bool AddFilter(Func filter) => CheckAndAddFilter(p => p.TransactionFilters.Add(filter)); + public static bool AddFilter(Func filter) + { + if (!IsConfigured) + { + TransactionFilters.Add(filter); + return true; + } + + return CheckAndAddFilter(p => p.AddFilter(filter)); + } /// /// Adds a filter which gets called before each span gets sent to APM Server. @@ -133,7 +155,16 @@ public static class Agent /// true if the filter was added successfully, false otherwise. In case the method /// returns false the filter won't be called. /// - public static bool AddFilter(Func filter) => CheckAndAddFilter(p => p.SpanFilters.Add(filter)); + public static bool AddFilter(Func filter) + { + if (!IsConfigured) + { + SpanFilters.Add(filter); + return true; + } + + return CheckAndAddFilter(p => p.AddFilter(filter)); + } /// /// Adds a filter which gets called before each error gets sent to APM Server. @@ -152,15 +183,23 @@ public static class Agent /// true if the filter was added successfully, false otherwise. In case the method /// returns false the filter won't be called. /// - public static bool AddFilter(Func filter) => CheckAndAddFilter(p => p.ErrorFilters.Add(filter)); + public static bool AddFilter(Func filter) + { + if (!IsConfigured) + { + ErrorFilters.Add(filter); + return true; + } - private static bool CheckAndAddFilter(Action action) + return CheckAndAddFilter(p => p.AddFilter(filter)); + } + + private static bool CheckAndAddFilter(Func action) { - if (!(Instance.PayloadSender is PayloadSenderV2 payloadSenderV2)) + if (Instance.PayloadSender is not IPayloadSenderWithFilters sender) return false; - action(payloadSenderV2); - return true; + return action(sender); } /// @@ -202,7 +241,7 @@ public static void Setup(AgentComponents agentComponents) agentComponents?.Logger?.Trace() ?.Log("Initialization - Agent.Setup called"); - Components = agentComponents; + // Force initialization var _ = LazyApmAgent.Value; } diff --git a/src/Elastic.Apm/Report/IPayloadSender.cs b/src/Elastic.Apm/Report/IPayloadSender.cs index fe97a8fc7..8533cebde 100644 --- a/src/Elastic.Apm/Report/IPayloadSender.cs +++ b/src/Elastic.Apm/Report/IPayloadSender.cs @@ -2,6 +2,8 @@ // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. // See the LICENSE file in the project root for more information +using System; +using System.Collections.Generic; using Elastic.Apm.Api; namespace Elastic.Apm.Report @@ -16,4 +18,11 @@ public interface IPayloadSender void QueueTransaction(ITransaction transaction); } + + public interface IPayloadSenderWithFilters + { + bool AddFilter(Func transactionFilter); + bool AddFilter(Func spanFilter); + bool AddFilter(Func errorFilter); + } } diff --git a/src/Elastic.Apm/Report/PayloadSenderV2.cs b/src/Elastic.Apm/Report/PayloadSenderV2.cs index ea94ab2c5..70661ca8b 100644 --- a/src/Elastic.Apm/Report/PayloadSenderV2.cs +++ b/src/Elastic.Apm/Report/PayloadSenderV2.cs @@ -29,12 +29,12 @@ namespace Elastic.Apm.Report /// Responsible for sending the data to APM server. Implements Intake V2. /// Each instance creates its own thread to do the work. Therefore, instances should be reused if possible. /// - internal class PayloadSenderV2 : BackendCommComponentBase, IPayloadSender + internal class PayloadSenderV2 : BackendCommComponentBase, IPayloadSender, IPayloadSenderWithFilters { private const string ThisClassName = nameof(PayloadSenderV2); - internal readonly List> ErrorFilters = new List>(); - internal readonly List> SpanFilters = new List>(); - internal readonly List> TransactionFilters = new List>(); + internal readonly List> ErrorFilters = new(); + internal readonly List> SpanFilters = new(); + internal readonly List> TransactionFilters = new(); private readonly IApmServerInfo _apmServerInfo; @@ -148,6 +148,7 @@ IApmLogger logger private bool _getApmServerVersion; private bool _getCloudMetadata; + private bool _allowFilterAdd; private static readonly UTF8Encoding Utf8Encoding; private static readonly MediaTypeHeaderValue MediaTypeHeaderValue; @@ -244,6 +245,8 @@ protected override void WorkLoopIteration() } var batch = ReceiveBatch(); + if (_allowFilterAdd && batch is { Length: > 0 }) + _allowFilterAdd = false; if (batch != null) ProcessQueueItems(batch); } @@ -497,5 +500,29 @@ private T TryExecuteFilter(List> filters, T item) where T : class return item; } + + public bool AddFilter(Func transactionFilter) + { + if (!_allowFilterAdd) return false; + + TransactionFilters.Add(transactionFilter); + return true; + } + + public bool AddFilter(Func spanFilter) + { + if (!_allowFilterAdd) return false; + + SpanFilters.Add(spanFilter); + return true; + } + + public bool AddFilter(Func errorFilter) + { + if (!_allowFilterAdd) return false; + + ErrorFilters.Add(errorFilter); + return true; + } } } diff --git a/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs b/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs index c72b7c21d..8899f457b 100644 --- a/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs +++ b/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs @@ -19,7 +19,7 @@ namespace Elastic.Apm.Tests.Utilities { - internal class MockPayloadSender : IPayloadSender + internal class MockPayloadSender : IPayloadSender, IPayloadSenderWithFilters { private static readonly JObject JsonSpanTypesData = JObject.Parse(File.ReadAllText(Path.Combine(SolutionPaths.Root, "test/Elastic.Apm.Tests.Utilities/TestResources/json-specs/span_types.json"))); @@ -211,7 +211,7 @@ public IReadOnlyList Metrics get { lock (_metricsLock) - return CreateImmutableSnapshot(_metrics); + return CreateImmutableSnapshot(_metrics); } } @@ -220,7 +220,7 @@ public IReadOnlyList Spans get { lock (_spanLock) - return CreateImmutableSnapshot(_spans); + return CreateImmutableSnapshot(_spans); } } @@ -229,7 +229,7 @@ public IReadOnlyList Transactions get { lock (_transactionLock) - return CreateImmutableSnapshot(_transactions); + return CreateImmutableSnapshot(_transactions); } } @@ -240,8 +240,8 @@ public void QueueError(IError error) { lock (_errorLock) { - error = _errorFilters.Aggregate(error, - (current, filter) => filter(current)); + error = _errorFilters + .Aggregate(error, (current, filter) => filter(current)); _errors.Add(error); _errorWaitHandle.Set(); } @@ -251,8 +251,8 @@ public virtual void QueueTransaction(ITransaction transaction) { lock (_transactionLock) { - transaction = _transactionFilters.Aggregate(transaction, - (current, filter) => filter(current)); + transaction = _transactionFilters + .Aggregate(transaction, (current, filter) => filter(current)); _transactions.Add(transaction); _transactionWaitHandle.Set(); } @@ -263,7 +263,8 @@ public void QueueSpan(ISpan span) VerifySpan(span); lock (_spanLock) { - span = _spanFilters.Aggregate(span, (current, filter) => filter(current)); + span = _spanFilters + .Aggregate(span, (current, filter) => filter(current)); _spans.Add(span); _spanWaitHandle.Set(); } @@ -305,6 +306,24 @@ private void VerifySpan(ISpan span) } } + public bool AddFilter(Func transactionFilter) + { + _transactionFilters.Add(transactionFilter); + return true; + } + + public bool AddFilter(Func spanFilter) + { + _spanFilters.Add(spanFilter); + return true; + } + + public bool AddFilter(Func errorFilter) + { + _errorFilters.Add(errorFilter); + return true; + } + public void QueueMetrics(IMetricSet metricSet) { lock (_metricsLock) diff --git a/test/Elastic.Apm.Tests.Utilities/NoopPayloadSender.cs b/test/Elastic.Apm.Tests.Utilities/NoopPayloadSender.cs index 7c75ce3aa..cbaaedea6 100644 --- a/test/Elastic.Apm.Tests.Utilities/NoopPayloadSender.cs +++ b/test/Elastic.Apm.Tests.Utilities/NoopPayloadSender.cs @@ -2,12 +2,14 @@ // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. // See the LICENSE file in the project root for more information +using System; +using System.Collections.Generic; using Elastic.Apm.Api; using Elastic.Apm.Report; namespace Elastic.Apm.Tests.Utilities { - public class NoopPayloadSender : IPayloadSender + public class NoopPayloadSender : IPayloadSender, IPayloadSenderWithFilters { public void QueueError(IError error) { } @@ -16,5 +18,12 @@ public void QueueTransaction(ITransaction transaction) { } public void QueueSpan(ISpan span) { } public void QueueMetrics(IMetricSet metricSet) { } + + public bool AddFilter(Func transactionFilter) => true; + + public bool AddFilter(Func spanFilter) => true; + + public bool AddFilter(Func errorFilter) => true; + } } diff --git a/test/Elastic.Apm.Tests/FilterTests.cs b/test/Elastic.Apm.Tests/FilterTests.cs index 8626237a1..8de289b30 100644 --- a/test/Elastic.Apm.Tests/FilterTests.cs +++ b/test/Elastic.Apm.Tests/FilterTests.cs @@ -44,13 +44,13 @@ public async Task RenameTransactionNameAndTypeIn2Filters() => await RegisterFilterRunCodeAndAssert( payloadSender => { - payloadSender.TransactionFilters.Add(t => + payloadSender.AddFilter((ITransaction t) => { t.Name = "NewTransactionName"; return t; }); - payloadSender.TransactionFilters.Add(t => + payloadSender.AddFilter((ITransaction t) => { t.Type = "NewTransactionType"; return t; @@ -78,17 +78,17 @@ await RegisterFilterRunCodeAndAssert( payloadSender => { // Rename transaction name - payloadSender.TransactionFilters.Add(t => + payloadSender.AddFilter((ITransaction t) => { t.Name = "NewTransactionName"; return t; }); // Throw an exception - payloadSender.TransactionFilters.Add(_ => throw new Exception("This is a test exception")); + payloadSender.AddFilter((ITransaction _) => throw new Exception("This is a test exception")); // Rename transaction type - payloadSender.TransactionFilters.Add(t => + payloadSender.AddFilter((ITransaction t) => { t.Type = "NewTransactionType"; return t; @@ -115,17 +115,17 @@ public async Task FilterSpanWith3Filters() payloadSender => { // Rename span name - payloadSender.SpanFilters.Add(span => + payloadSender.AddFilter((ISpan span) => { span.Name = "NewSpanName"; return span; }); // Throw an exception - payloadSender.SpanFilters.Add(_ => throw new Exception("This is a test exception")); + payloadSender.AddFilter((ISpan _) => throw new Exception("This is a test exception")); // Rename span type - payloadSender.SpanFilters.Add(span => + payloadSender.AddFilter((ISpan span) => { span.Type = "NewSpanType"; return span; @@ -156,7 +156,7 @@ public async Task DropSpanWithSpecificName() payloadSender => { // Rename span name - payloadSender.SpanFilters.Add(span => + payloadSender.AddFilter((ISpan span) => { if (span.Name == "SpanToDrop") return null; @@ -175,7 +175,28 @@ public async Task DropSpanWithSpecificName() return true; }); - private async Task RegisterFilterRunCodeAndAssert(Action registerFilters, Action executeCodeThatGeneratesData, + /// + /// Registers a span-filter that returns false for specific span names and sends a span with that specific name. + /// Makes sure the span is not sent and serialized. + /// + [Fact] + public async Task FilterDoesNotBReak() + => await RegisterFilterRunCodeAndAssert( + payloadSender => + { + payloadSender.AddFilter((ISpan span) => span); + }, + agent => + { + agent.Tracer.CaptureTransaction("Test123", "TestTransaction", + t => { t.CaptureSpan("SpanToDrop", "TestSpan", () => Thread.Sleep(10)); }); + }, + (_, spans, _) => + { + return true; + }); + + private async Task RegisterFilterRunCodeAndAssert(Action registerFilters, Action executeCodeThatGeneratesData, Func, List, List, bool> assert ) { From 00192c18a117836dbed5ef8f2cc40d2e135ef167 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Thu, 15 Aug 2024 14:17:14 +0200 Subject: [PATCH 2/3] dotnet format --- src/Elastic.Apm/Agent.cs | 3 ++- src/Elastic.Apm/Report/PayloadSenderV2.cs | 9 ++++++--- test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Elastic.Apm/Agent.cs b/src/Elastic.Apm/Agent.cs index da85e8a5a..8dcb10b61 100644 --- a/src/Elastic.Apm/Agent.cs +++ b/src/Elastic.Apm/Agent.cs @@ -74,7 +74,8 @@ public static class Agent agent.Logger?.Trace() ?.Log("Initialization - Agent instance initialized. Callstack: {callstack}", new StackTrace().ToString()); - if (agent.Components.PayloadSender is not IPayloadSenderWithFilters sender) return agent; + if (agent.Components.PayloadSender is not IPayloadSenderWithFilters sender) + return agent; ErrorFilters.ForEach(f => sender.AddFilter(f)); TransactionFilters.ForEach(f => sender.AddFilter(f)); diff --git a/src/Elastic.Apm/Report/PayloadSenderV2.cs b/src/Elastic.Apm/Report/PayloadSenderV2.cs index 70661ca8b..9c5b074b0 100644 --- a/src/Elastic.Apm/Report/PayloadSenderV2.cs +++ b/src/Elastic.Apm/Report/PayloadSenderV2.cs @@ -503,7 +503,8 @@ private T TryExecuteFilter(List> filters, T item) where T : class public bool AddFilter(Func transactionFilter) { - if (!_allowFilterAdd) return false; + if (!_allowFilterAdd) + return false; TransactionFilters.Add(transactionFilter); return true; @@ -511,7 +512,8 @@ public bool AddFilter(Func transactionFilter) public bool AddFilter(Func spanFilter) { - if (!_allowFilterAdd) return false; + if (!_allowFilterAdd) + return false; SpanFilters.Add(spanFilter); return true; @@ -519,7 +521,8 @@ public bool AddFilter(Func spanFilter) public bool AddFilter(Func errorFilter) { - if (!_allowFilterAdd) return false; + if (!_allowFilterAdd) + return false; ErrorFilters.Add(errorFilter); return true; diff --git a/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs b/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs index 8899f457b..01df74d34 100644 --- a/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs +++ b/test/Elastic.Apm.Tests.Utilities/MockPayloadSender.cs @@ -251,7 +251,7 @@ public virtual void QueueTransaction(ITransaction transaction) { lock (_transactionLock) { - transaction = _transactionFilters + transaction = _transactionFilters .Aggregate(transaction, (current, filter) => filter(current)); _transactions.Add(transaction); _transactionWaitHandle.Set(); From 6cfc254a3cc60b0ce29583c58eacfddd1a1ab0fd Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Thu, 15 Aug 2024 16:13:14 +0200 Subject: [PATCH 3/3] ensure we assign to components again --- src/Elastic.Apm/Agent.cs | 2 ++ .../Elastic.Apm.AspNetCore.Static.Tests/ConfigTests.cs | 10 +++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Elastic.Apm/Agent.cs b/src/Elastic.Apm/Agent.cs index 8dcb10b61..e32a5ded7 100644 --- a/src/Elastic.Apm/Agent.cs +++ b/src/Elastic.Apm/Agent.cs @@ -239,6 +239,8 @@ public static void Setup(AgentComponents agentComponents) return; } + Components ??= agentComponents; + agentComponents?.Logger?.Trace() ?.Log("Initialization - Agent.Setup called"); diff --git a/test/integrations/Elastic.Apm.AspNetCore.Static.Tests/ConfigTests.cs b/test/integrations/Elastic.Apm.AspNetCore.Static.Tests/ConfigTests.cs index 530881891..12d439c7d 100644 --- a/test/integrations/Elastic.Apm.AspNetCore.Static.Tests/ConfigTests.cs +++ b/test/integrations/Elastic.Apm.AspNetCore.Static.Tests/ConfigTests.cs @@ -10,6 +10,7 @@ using Elastic.Apm.AspNetCore.Tests; using Elastic.Apm.BackendComm.CentralConfig; using Elastic.Apm.Extensions.Hosting.Config; +using Elastic.Apm.Logging; using Elastic.Apm.Tests.Utilities; using Elastic.Apm.Tests.Utilities.XUnit; using FluentAssertions; @@ -61,9 +62,11 @@ public async Task AgentDisabledInAppConfig() var capturedPayload = new MockPayloadSender(); using var agent = new ApmAgent(new TestAgentComponents( - new NoopLogger(), + new XUnitLogger(LogLevel.Trace, TestOutputHelper), new RuntimeConfigurationSnapshot(configReader))); + agent.Configuration.Enabled.Should().BeFalse(); + var client = Helper.ConfigureHttpClient(true, agent, _factory); Agent.Setup(agent); @@ -72,13 +75,14 @@ public async Task AgentDisabledInAppConfig() var isParsed = bool.TryParse(await response.Content.ReadAsStringAsync(), out var boolVal); + // Make the test fail if there was a connection to the server URL made with the sample app's url + defaultServerUrlConnectionMade.Should().BeFalse(); + isParsed.Should().BeTrue(); boolVal.Should().BeFalse(); capturedPayload.Transactions.Should().BeNullOrEmpty(); capturedPayload.Spans.Should().BeNullOrEmpty(); capturedPayload.Errors.Should().BeNullOrEmpty(); - // Make the test fail if there was a connection to the server URL made with the sample app's url - defaultServerUrlConnectionMade.Should().BeFalse(); } } }