From f22c2d479028cc883bf20dc58828a312856bb2dc Mon Sep 17 00:00:00 2001 From: Stefan Jandl Date: Tue, 14 May 2024 14:05:14 +0200 Subject: [PATCH] revert: Tracing for `SentryHttpMessageHandler` with no active transaction (#3367) --- CHANGELOG.md | 6 + src/Sentry/HubExtensions.cs | 9 -- src/Sentry/{Internal => }/IMetricHub.cs | 15 ++- src/Sentry/Internal/Hub.cs | 10 ++ src/Sentry/SentryGraphQLHttpMessageHandler.cs | 2 +- src/Sentry/SentryHttpMessageHandler.cs | 3 +- src/Sentry/Timing.cs | 1 - .../SentryGraphQlHttpMessageHandlerTests.cs | 29 +++-- .../SentryHttpMessageHandlerTests.cs | 104 ++++++++---------- .../Sentry.Tests/SentryMessageHandlerTests.cs | 31 ------ test/Sentry.Tests/TimingTests.cs | 8 +- 11 files changed, 90 insertions(+), 128 deletions(-) rename src/Sentry/{Internal => }/IMetricHub.cs (52%) delete mode 100644 test/Sentry.Tests/SentryMessageHandlerTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a58c75be9..cb5029851b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Reverted changes to the SentryHttpMessageHandler and SentryGraphQLHttpMessageHandler to automatically create transactions for each request as this could negatively affect users' quota ([#3367](https://github.com/getsentry/sentry-dotnet/pull/3367)) + ## 4.6.1 ### Fixes diff --git a/src/Sentry/HubExtensions.cs b/src/Sentry/HubExtensions.cs index bb49fd9d03..a4bedfddec 100644 --- a/src/Sentry/HubExtensions.cs +++ b/src/Sentry/HubExtensions.cs @@ -244,13 +244,4 @@ internal static ITransactionTracer StartTransaction( var transaction = hub.GetTransaction(); return transaction?.IsSampled == true ? transaction : null; } - - internal static ISpan StartSpan(this IHub hub, string operation, string description) - { - ITransactionTracer? currentTransaction = null; - hub.ConfigureScope(s => currentTransaction = s.Transaction); - return currentTransaction is { } transaction - ? transaction.StartChild(operation, description) - : hub.StartTransaction(description, operation, description); - } } diff --git a/src/Sentry/Internal/IMetricHub.cs b/src/Sentry/IMetricHub.cs similarity index 52% rename from src/Sentry/Internal/IMetricHub.cs rename to src/Sentry/IMetricHub.cs index dba0db53c6..99f9d7d778 100644 --- a/src/Sentry/Internal/IMetricHub.cs +++ b/src/Sentry/IMetricHub.cs @@ -1,11 +1,8 @@ using Sentry.Protocol.Metrics; -namespace Sentry.Internal; +namespace Sentry; -/// -/// Specifies various internal methods required on the Hub for metrics to work. -/// -internal interface IMetricHub : IHub +internal interface IMetricHub { /// /// Captures one or more metrics to be sent to Sentry. @@ -16,4 +13,12 @@ internal interface IMetricHub : IHub /// Captures one or more to be sent to Sentry. /// void CaptureCodeLocations(CodeLocations codeLocations); + + /// + /// Starts a child span for the current transaction or, if there is no active transaction, starts a new transaction. + /// + ISpan StartSpan(string operation, string description); + + /// + ISpan? GetSpan(); } diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs index 47ece18584..1d3f92ae9d 100644 --- a/src/Sentry/Internal/Hub.cs +++ b/src/Sentry/Internal/Hub.cs @@ -530,6 +530,16 @@ public void CaptureCodeLocations(CodeLocations codeLocations) } } + /// + public ISpan StartSpan(string operation, string description) + { + ITransactionTracer? currentTransaction = null; + ConfigureScope(s => currentTransaction = s.Transaction); + return currentTransaction is { } transaction + ? transaction.StartChild(operation, description) + : this.StartTransaction(operation, description); + } + public void CaptureSession(SessionUpdate sessionUpdate) { if (!IsEnabled) diff --git a/src/Sentry/SentryGraphQLHttpMessageHandler.cs b/src/Sentry/SentryGraphQLHttpMessageHandler.cs index 828bf7e452..e9e98abe83 100644 --- a/src/Sentry/SentryGraphQLHttpMessageHandler.cs +++ b/src/Sentry/SentryGraphQLHttpMessageHandler.cs @@ -50,7 +50,7 @@ internal SentryGraphQLHttpMessageHandler(IHub? hub, SentryOptions? options, // Start a span that tracks this request // (may be null if transaction is not set on the scope) - var span = _hub.StartSpan( + var span = _hub.GetSpan()?.StartChild( "http.client", $"{method} {url}" // e.g. "GET https://example.com" ); diff --git a/src/Sentry/SentryHttpMessageHandler.cs b/src/Sentry/SentryHttpMessageHandler.cs index 557f192d95..fb037c853b 100644 --- a/src/Sentry/SentryHttpMessageHandler.cs +++ b/src/Sentry/SentryHttpMessageHandler.cs @@ -1,5 +1,4 @@ using Sentry.Extensibility; -using Sentry.Internal; using Sentry.Internal.OpenTelemetry; namespace Sentry; @@ -64,7 +63,7 @@ internal SentryHttpMessageHandler(IHub? hub, SentryOptions? options, HttpMessage { // Start a span that tracks this request // (may be null if transaction is not set on the scope) - var span = _hub.StartSpan( + var span = _hub.GetSpan()?.StartChild( "http.client", $"{method} {url}" // e.g. "GET https://example.com" ); diff --git a/src/Sentry/Timing.cs b/src/Sentry/Timing.cs index ea474fde5b..b979b99d0d 100644 --- a/src/Sentry/Timing.cs +++ b/src/Sentry/Timing.cs @@ -1,5 +1,4 @@ using Sentry.Extensibility; -using Sentry.Internal; using Sentry.Protocol.Metrics; namespace Sentry; diff --git a/test/Sentry.Tests/SentryGraphQlHttpMessageHandlerTests.cs b/test/Sentry.Tests/SentryGraphQlHttpMessageHandlerTests.cs index 304b903e22..a545f501d1 100644 --- a/test/Sentry.Tests/SentryGraphQlHttpMessageHandlerTests.cs +++ b/test/Sentry.Tests/SentryGraphQlHttpMessageHandlerTests.cs @@ -7,7 +7,7 @@ namespace Sentry.Tests; * TODO: Find a way to consolidate these tests cleanly. */ -public class SentryGraphQlHttpMessageHandlerTests : SentryMessageHandlerTests +public class SentryGraphQlHttpMessageHandlerTests { private const string ValidQuery = "query getAllNotes { notes { id } }"; private const string ValidResponse = @"{ @@ -47,7 +47,13 @@ public void ProcessRequest_ExtractsGraphQlRequestContent() public void ProcessRequest_SetsSpanData() { // Arrange - var hub = _fixture.GetHub(); + var hub = Substitute.For(); + var parentSpan = Substitute.For(); + hub.GetSpan().Returns(parentSpan); + var childSpan = Substitute.For(); + parentSpan.When(p => p.StartChild(Arg.Any())) + .Do(op => childSpan.Operation = op.Arg()); + parentSpan.StartChild(Arg.Any()).Returns(childSpan); var sut = new SentryGraphQLHttpMessageHandler(hub, null); var method = "POST"; @@ -61,19 +67,9 @@ public void ProcessRequest_SetsSpanData() // Assert returnedSpan.Should().NotBeNull(); - using (new AssertionScope()) - { - returnedSpan!.Operation.Should().Be("http.client"); - returnedSpan.Description.Should().Be($"{method} {url}"); - returnedSpan.Extra.Should().Contain(kvp => - kvp.Key == OtelSemanticConventions.AttributeHttpRequestMethod && - kvp.Value.ToString() == method - ); - returnedSpan.Extra.Should().Contain(kvp => - kvp.Key == OtelSemanticConventions.AttributeServerAddress && - kvp.Value.ToString() == host - ); - } + returnedSpan!.Operation.Should().Be("http.client"); + returnedSpan.Description.Should().Be($"{method} {url}"); + returnedSpan.Received(1).SetExtra(OtelSemanticConventions.AttributeHttpRequestMethod, method); } // [Theory] @@ -123,13 +119,14 @@ public void HandleResponse_AddsBreadcrumb() public void HandleResponse_SetsSpanData() { // Arrange - var hub = _fixture.GetHub(); + var hub = Substitute.For(); var status = HttpStatusCode.OK; var response = new HttpResponseMessage(status); var method = "POST"; var url = "http://example.com/graphql"; var request = SentryGraphQlTestHelpers.GetRequestQuery(ValidQuery, url); response.RequestMessage = request; + hub.GetSpan().Returns(new TransactionTracer(hub, "test", "test")); var sut = new SentryGraphQLHttpMessageHandler(hub, null); // Act diff --git a/test/Sentry.Tests/SentryHttpMessageHandlerTests.cs b/test/Sentry.Tests/SentryHttpMessageHandlerTests.cs index 4e91ecab14..fc38202019 100644 --- a/test/Sentry.Tests/SentryHttpMessageHandlerTests.cs +++ b/test/Sentry.Tests/SentryHttpMessageHandlerTests.cs @@ -7,7 +7,7 @@ namespace Sentry.Tests; * TODO: Find a way to consolidate these tests cleanly. */ -public class SentryHttpMessageHandlerTests : SentryMessageHandlerTests +public class SentryHttpMessageHandlerTests { [Fact] public async Task SendAsync_SentryTraceHeaderNotSet_SetsHeader_ByDefault() @@ -122,12 +122,17 @@ public async Task SendAsync_SentryTraceHeaderAlreadySet_NotOverwritten() } [Fact] - public async Task SendAsync_TransactionOnScope_StartsChildSpan() + public async Task SendAsync_TransactionOnScope_StartsNewSpan() { // Arrange - var hub = _fixture.GetHub(); - var transaction = hub.StartTransaction("foo", "bar"); - hub.ConfigureScope(scope => scope.Transaction = transaction); + var hub = Substitute.For(); + + var transaction = new TransactionTracer( + hub, + "foo", + "bar"); + + hub.GetSpan().ReturnsForAnyArgs(transaction); using var innerHandler = new FakeHttpMessageHandler(); using var sentryHandler = new SentryHttpMessageHandler(innerHandler, hub); @@ -144,40 +149,17 @@ public async Task SendAsync_TransactionOnScope_StartsChildSpan() } [Fact] - public async Task SendAsync_NoTransactionOnScope_StartsTransaction() + public async Task SendAsync_ExceptionThrown_ExceptionLinkedToSpan() { // Arrange - SentryTransaction received = null; - _fixture.Client.CaptureTransaction( - Arg.Do(t => received = t), - Arg.Any(), - Arg.Any() - ); - var hub = _fixture.GetHub(); - - using var innerHandler = new FakeHttpMessageHandler(); - using var sentryHandler = new SentryHttpMessageHandler(innerHandler, hub); - using var client = new HttpClient(sentryHandler); + var hub = Substitute.For(); - // Act - await client.GetAsync("https://localhost/"); + var transaction = new TransactionTracer( + hub, + "foo", + "bar"); - // Assert - received.Should().NotBeNull(); - using (new AssertionScope()) - { - received.Name.Should().Be("GET https://localhost/"); - received.Operation.Should().Be("http.client"); - received.Description.Should().Be("GET https://localhost/"); - received.IsFinished.Should().BeTrue(); - } - } - - [Fact] - public async Task SendAsync_ExceptionThrown_ExceptionLinkedToSpan() - { - // Arrange - var hub = _fixture.GetHub(); + hub.GetSpan().ReturnsForAnyArgs(transaction); var exception = new Exception(); @@ -189,8 +171,7 @@ public async Task SendAsync_ExceptionThrown_ExceptionLinkedToSpan() await Assert.ThrowsAsync(() => client.GetAsync("https://localhost/")); // Assert - hub.ExceptionToSpanMap.TryGetValue(exception, out var span).Should().BeTrue(); - span.Should().NotBeNull(); + hub.Received(1).BindException(exception, Arg.Any()); // second argument is an implicitly created span } [Fact] @@ -262,15 +243,18 @@ public async Task SendAsync_Executed_FailedRequestsCaptured() public void ProcessRequest_SetsSpanData() { // Arrange - var hub = _fixture.GetHub(); - using var innerHandler = new FakeHttpMessageHandler(); - var sut = new SentryHttpMessageHandler(hub, _fixture.Options, innerHandler); + var hub = Substitute.For(); + var parentSpan = Substitute.For(); + hub.GetSpan().Returns(parentSpan); + var childSpan = Substitute.For(); + parentSpan.When(p => p.StartChild(Arg.Any())) + .Do(op => childSpan.Operation = op.Arg()); + parentSpan.StartChild(Arg.Any()).Returns(childSpan); + var sut = new SentryHttpMessageHandler(hub, null); var method = "GET"; - var host = "example.com"; - var url = $"https://{host}/graphql"; - var uri = new Uri(url); - var request = new HttpRequestMessage(HttpMethod.Get, uri); + var url = "http://example.com/graphql"; + var request = new HttpRequestMessage(HttpMethod.Get, url); // Act var returnedSpan = sut.ProcessRequest(request, method, url); @@ -279,14 +263,7 @@ public void ProcessRequest_SetsSpanData() returnedSpan.Should().NotBeNull(); returnedSpan!.Operation.Should().Be("http.client"); returnedSpan.Description.Should().Be($"{method} {url}"); - returnedSpan.Extra.Should().Contain(kvp => - kvp.Key == OtelSemanticConventions.AttributeHttpRequestMethod && - Equals(kvp.Value, method) - ); - returnedSpan.Extra.Should().Contain(kvp => - kvp.Key == OtelSemanticConventions.AttributeServerAddress && - Equals(kvp.Value, host) - ); + returnedSpan.Received(1).SetExtra(OtelSemanticConventions.AttributeHttpRequestMethod, method); } [Fact] @@ -430,9 +407,14 @@ public void Send_SentryTraceHeaderAlreadySet_NotOverwritten() public void Send_TransactionOnScope_StartsNewSpan() { // Arrange - var hub = _fixture.GetHub(); - var transaction = hub.StartTransaction("foo", "bar"); - hub.ConfigureScope(scope => scope.Transaction = transaction); + var hub = Substitute.For(); + + var transaction = new TransactionTracer( + hub, + "foo", + "bar"); + + hub.GetSpan().ReturnsForAnyArgs(transaction); using var innerHandler = new FakeHttpMessageHandler(); using var sentryHandler = new SentryHttpMessageHandler(innerHandler, hub); @@ -452,7 +434,14 @@ public void Send_TransactionOnScope_StartsNewSpan() public void Send_ExceptionThrown_ExceptionLinkedToSpan() { // Arrange - var hub = _fixture.GetHub(); + var hub = Substitute.For(); + + var transaction = new TransactionTracer( + hub, + "foo", + "bar"); + + hub.GetSpan().ReturnsForAnyArgs(transaction); var exception = new Exception(); @@ -464,8 +453,7 @@ public void Send_ExceptionThrown_ExceptionLinkedToSpan() Assert.Throws(() => client.Get("https://localhost/")); // Assert - hub.ExceptionToSpanMap.TryGetValue(exception, out var span).Should().BeTrue(); - span.Should().NotBeNull(); + hub.Received(1).BindException(exception, Arg.Any()); // second argument is an implicitly created span } [Fact] diff --git a/test/Sentry.Tests/SentryMessageHandlerTests.cs b/test/Sentry.Tests/SentryMessageHandlerTests.cs deleted file mode 100644 index 24152d985a..0000000000 --- a/test/Sentry.Tests/SentryMessageHandlerTests.cs +++ /dev/null @@ -1,31 +0,0 @@ -using Sentry.Internal.OpenTelemetry; - -namespace Sentry.Tests; - -public class SentryMessageHandlerTests -{ - private protected class Fixture - { - public readonly SentryOptions Options = new(); - - public readonly ISentryClient Client = Substitute.For(); - - public readonly ISessionManager SessionManager = Substitute.For(); - - public readonly ISystemClock Clock = Substitute.For(); - - public Fixture() - { - Options.Dsn = ValidDsn; - Options.TracesSampleRate = 1.0; - } - - public Hub GetHub() - { - var scopeManager = new SentryScopeManager(Options, Client); - return new Hub(Options, Client, SessionManager, Clock, scopeManager); - } - } - - private protected readonly Fixture _fixture = new(); -} diff --git a/test/Sentry.Tests/TimingTests.cs b/test/Sentry.Tests/TimingTests.cs index 8d6ba6c09f..f203be15f9 100644 --- a/test/Sentry.Tests/TimingTests.cs +++ b/test/Sentry.Tests/TimingTests.cs @@ -41,16 +41,14 @@ public void Constructor_CreatesSpan() // Arrange _fixture.Tags = new Dictionary { { "tag1", "value1" } }; - var span = new TransactionTracer(_fixture.Hub, _fixture.Key, Timing.OperationName); - _fixture.MetricHub.StartTransaction( - Arg.Any(), - Arg.Any>() - ).Returns(span); + var span = new TransactionTracer(_fixture.Hub, Timing.OperationName, _fixture.Key); + _fixture.MetricHub.StartSpan(Timing.OperationName, _fixture.Key).Returns(span); // Act _ = _fixture.GetSut(); // Assert + _fixture.MetricHub.Received(1).StartSpan(Timing.OperationName, _fixture.Key); span.Tags.Should().BeEquivalentTo(_fixture.Tags); }