From 8e99d01ee4b839aac1587edbfc2d31eb4629def8 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Thu, 15 Aug 2024 17:19:14 +0200 Subject: [PATCH] Adding filters should not force initialization of Agent. (#2418) 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 | 62 ++++++++++++++++--- src/Elastic.Apm/Report/IPayloadSender.cs | 9 +++ src/Elastic.Apm/Report/PayloadSenderV2.cs | 38 ++++++++++-- .../MockPayloadSender.cs | 37 ++++++++--- .../NoopPayloadSender.cs | 11 +++- test/Elastic.Apm.Tests/FilterTests.cs | 41 +++++++++--- .../ConfigTests.cs | 10 ++- 7 files changed, 171 insertions(+), 37 deletions(-) diff --git a/src/Elastic.Apm/Agent.cs b/src/Elastic.Apm/Agent.cs index b5630b16d..e32a5ded7 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,31 @@ 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 +128,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 +156,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 +184,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; + } + + return CheckAndAddFilter(p => p.AddFilter(filter)); + } - private static bool CheckAndAddFilter(Action action) + 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); } /// @@ -199,10 +239,12 @@ public static void Setup(AgentComponents agentComponents) return; } + Components ??= 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..9c5b074b0 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,32 @@ 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..01df74d34 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 ) { 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(); } } }