revert: Tracing for SentryHttpMessageHandler with no active transac…

…tion (#3367)
getsentry · May 14, 2024 · f22c2d4 · f22c2d4
1 parent acbbdd3
commit f22c2d4
Show file tree

Hide file tree

Showing 11 changed files with 90 additions and 128 deletions.
diff --git 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
@@ -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 → src/Sentry/IMetricHub.cs b/src/Sentry/Internal/IMetricHub.cs → src/Sentry/IMetricHub.cs
@@ -1,11 +1,8 @@
 using Sentry.Protocol.Metrics;
 
-namespace Sentry.Internal;
+namespace Sentry;
 
-/// <summary>
-/// Specifies various internal methods required on the Hub for metrics to work.
-/// </summary>
-internal interface IMetricHub : IHub
+internal interface IMetricHub
 {
     /// <summary>
     /// Captures one or more metrics to be sent to Sentry.
@@ -16,4 +13,12 @@ internal interface IMetricHub : IHub
     /// Captures one or more <see cref="CodeLocations"/> to be sent to Sentry.
     /// </summary>
     void CaptureCodeLocations(CodeLocations codeLocations);
+
+    /// <summary>
+    /// Starts a child span for the current transaction or, if there is no active transaction, starts a new transaction.
+    /// </summary>
+    ISpan StartSpan(string operation, string description);
+
+    /// <inheritdoc cref="IHub.GetSpan"/>
+    ISpan? GetSpan();
 }
diff --git a/src/Sentry/Internal/Hub.cs b/src/Sentry/Internal/Hub.cs
@@ -530,6 +530,16 @@ public void CaptureCodeLocations(CodeLocations codeLocations)
         }
     }
 
+    /// <inheritdoc cref="IMetricHub.StartSpan"/>
+    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
@@ -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
@@ -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
@@ -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
@@ -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<IHub>();
+        var parentSpan = Substitute.For<ISpan>();
+        hub.GetSpan().Returns(parentSpan);
+        var childSpan = Substitute.For<ISpan>();
+        parentSpan.When(p => p.StartChild(Arg.Any<string>()))
+                  .Do(op => childSpan.Operation = op.Arg<string>());
+        parentSpan.StartChild(Arg.Any<string>()).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<IHub>();
         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
@@ -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<IHub>();
+
+        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<SentryTransaction>(t => received = t),
-            Arg.Any<Scope>(),
-            Arg.Any<SentryHint>()
-        );
-        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<IHub>();
 
-        // 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<Exception>(() => client.GetAsync("https://localhost/"));
 
         // Assert
-        hub.ExceptionToSpanMap.TryGetValue(exception, out var span).Should().BeTrue();
-        span.Should().NotBeNull();
+        hub.Received(1).BindException(exception, Arg.Any<ISpan>()); // 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<IHub>();
+        var parentSpan = Substitute.For<ISpan>();
+        hub.GetSpan().Returns(parentSpan);
+        var childSpan = Substitute.For<ISpan>();
+        parentSpan.When(p => p.StartChild(Arg.Any<string>()))
+            .Do(op => childSpan.Operation = op.Arg<string>());
+        parentSpan.StartChild(Arg.Any<string>()).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<IHub>();
+
+        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<IHub>();
+
+        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<Exception>(() => client.Get("https://localhost/"));
 
         // Assert
-        hub.ExceptionToSpanMap.TryGetValue(exception, out var span).Should().BeTrue();
-        span.Should().NotBeNull();
+        hub.Received(1).BindException(exception, Arg.Any<ISpan>()); // second argument is an implicitly created span
     }
 
     [Fact]

diff --git a/test/Sentry.Tests/SentryMessageHandlerTests.cs b/test/Sentry.Tests/SentryMessageHandlerTests.cs
diff --git 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<string, string> { { "tag1", "value1" } };
 
-        var span = new TransactionTracer(_fixture.Hub, _fixture.Key, Timing.OperationName);
-        _fixture.MetricHub.StartTransaction(
-            Arg.Any<TransactionContext>(),
-            Arg.Any<IReadOnlyDictionary<string, object>>()
-            ).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);
     }