Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Tracing without performance #2493

Merged
merged 49 commits into from
Aug 3, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
d486409
wip
bitsandfoxes Jul 18, 2023
3ec17d7
added propagation context
bitsandfoxes Jul 18, 2023
60dabd0
baggage fetching
bitsandfoxes Jul 19, 2023
8c90704
scope creates propagationcontext
bitsandfoxes Jul 19, 2023
3f88d89
http handler retrieves baggage from hub
bitsandfoxes Jul 19, 2023
56f14d9
keeping things internal
bitsandfoxes Jul 19, 2023
6fecb32
properly propagating things with tracing disabled
bitsandfoxes Jul 19, 2023
84971ed
wip
bitsandfoxes Jul 20, 2023
6c7aff1
simplifying
bitsandfoxes Jul 20, 2023
717b37e
caching parsed DSN
bitsandfoxes Jul 24, 2023
16494c6
reviewpoints
bitsandfoxes Jul 25, 2023
0a37731
continuing trace in ASP.NET Core
bitsandfoxes Jul 25, 2023
c6d632c
simplified
bitsandfoxes Jul 25, 2023
9f94934
simplified 2
bitsandfoxes Jul 25, 2023
b421c88
merged main
bitsandfoxes Jul 25, 2023
f4a6b04
cleanup
bitsandfoxes Jul 25, 2023
848af4e
fixed tracing middleware
bitsandfoxes Jul 25, 2023
dc0e022
wip
bitsandfoxes Jul 26, 2023
b973357
tests
bitsandfoxes Jul 28, 2023
522a752
Apply suggestions from code review
bitsandfoxes Jul 31, 2023
cb54916
objects as keys
bitsandfoxes Jul 31, 2023
7b54760
merge
bitsandfoxes Jul 31, 2023
d87e4d5
fixed using
bitsandfoxes Jul 31, 2023
cd02bce
logging the name of the missing key
bitsandfoxes Jul 31, 2023
d27622d
cleanup
bitsandfoxes Jul 31, 2023
6bfd515
comments
bitsandfoxes Jul 31, 2023
ffc633a
merged main
bitsandfoxes Jul 31, 2023
91b3bd1
renamed internal 'IsTracingEnabled' to 'IsPerformanceMonitoringEnabled'
bitsandfoxes Jul 31, 2023
3ed5a8f
test
bitsandfoxes Jul 31, 2023
dfc4274
istracingenabled
bitsandfoxes Jul 31, 2023
b812630
removed unused method
bitsandfoxes Jul 31, 2023
9640278
fixed parsing dsn test
bitsandfoxes Jul 31, 2023
57ff2f5
last one? maybe?
bitsandfoxes Aug 1, 2023
dd1ed93
added release to options in verify
bitsandfoxes Aug 2, 2023
48ba10a
updated verify
bitsandfoxes Aug 2, 2023
75916f0
simplified
bitsandfoxes Aug 2, 2023
fde8865
reverted submodule bump
bitsandfoxes Aug 2, 2023
ee2a0c8
disabled hub
bitsandfoxes Aug 2, 2023
ced245b
Updated CHANGELOG.md
bitsandfoxes Aug 2, 2023
5c3b22e
setting release
bitsandfoxes Aug 2, 2023
cc049ad
missed a release
bitsandfoxes Aug 2, 2023
f072aad
trying to fix the verified
bitsandfoxes Aug 2, 2023
3cb0af4
line numbers
bitsandfoxes Aug 2, 2023
3dd2265
last round of verified?
bitsandfoxes Aug 2, 2023
60d1f1c
reset
bitsandfoxes Aug 2, 2023
b5afd16
dear lord.. please accept these verified update
bitsandfoxes Aug 2, 2023
88ba20c
added missing mono
bitsandfoxes Aug 2, 2023
bde03c6
whitespace?
bitsandfoxes Aug 2, 2023
139579a
Apply suggestions from code review
bitsandfoxes Aug 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/sentry-cocoa
Submodule sentry-cocoa updated 267 files
28 changes: 23 additions & 5 deletions src/Sentry.AspNet/HttpContextExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,25 @@ public static class HttpContextExtensions
}
}

/// <summary>
/// Starts or continues a Sentry trace.
/// </summary>
public static void StartOrContinueTrace(this HttpContext httpContext)
{
var options = SentrySdk.CurrentOptions;

var traceHeader = TryGetSentryTraceHeader(httpContext, options);
var baggageHeader = TryGetBaggageHeader(httpContext, options);

var method = httpContext.Request.HttpMethod;
var path = httpContext.Request.Path;

var transactionName = $"{method} {path}";
const string operation = "http.server";

SentrySdk.ContinueTrace(traceHeader, baggageHeader, transactionName, operation);
}

/// <summary>
/// Starts a new Sentry transaction that encompasses the currently executing HTTP request.
/// </summary>
Expand All @@ -65,13 +84,13 @@ public static ITransaction StartSentryTransaction(this HttpContext httpContext)
var options = SentrySdk.CurrentOptions;

var traceHeader = TryGetSentryTraceHeader(httpContext, options);
var baggageHeader = TryGetBaggageHeader(httpContext, options);

var transactionName = $"{method} {path}";
const string transactionOperation = "http.server";

var transactionContext = traceHeader is not null
? new TransactionContext(transactionName, transactionOperation, traceHeader, TransactionNameSource.Url)
: new TransactionContext(transactionName, transactionOperation, TransactionNameSource.Url);
var transactionContext = SentrySdk.ContinueTrace(traceHeader, baggageHeader, transactionName, transactionOperation);
transactionContext.NameSource = TransactionNameSource.Url;

var customSamplingContext = new Dictionary<string, object?>(3, StringComparer.Ordinal)
{
Expand All @@ -81,10 +100,9 @@ public static ITransaction StartSentryTransaction(this HttpContext httpContext)
};

// Set the Dynamic Sampling Context from the baggage header, if it exists.
var baggageHeader = TryGetBaggageHeader(httpContext, options);
var dynamicSamplingContext = baggageHeader?.CreateDynamicSamplingContext();

if (traceHeader is { } && baggageHeader is null)
if (traceHeader is not null && baggageHeader is null)
{
// We received a sentry-trace header without a baggage header, which indicates the request
// originated from an older SDK that doesn't support dynamic sampling.
Expand Down
14 changes: 14 additions & 0 deletions src/Sentry.AspNetCore/SentryMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Sentry.AspNetCore.Extensions;
using Sentry.Extensibility;
using Sentry.Reflection;

Expand All @@ -17,6 +18,10 @@ namespace Sentry.AspNetCore;
/// </summary>
internal class SentryMiddleware : IMiddleware
{
internal static readonly object TraceHeaderItemKey = new();
internal static readonly object BaggageHeaderItemKey = new();
internal static readonly object TransactionContextItemKey = new();

private readonly Func<IHub> _getHub;
private readonly SentryAspNetCoreOptions _options;
private readonly IHostingEnvironment _hostingEnvironment;
Expand Down Expand Up @@ -90,6 +95,15 @@ public async Task InvokeAsync(HttpContext context, RequestDelegate next)
context.Response.OnCompleted(() => hub.FlushAsync(_options.FlushTimeout));
}

var traceHeader = context.TryGetSentryTraceHeader(_options);
var baggageHeader = context.TryGetBaggageHeader(_options);
var transactionContext = hub.ContinueTrace(traceHeader, baggageHeader);

// Adding the headers and the TransactionContext to the context to be picked up by the Sentry tracing middleware
context.Items.Add(TraceHeaderItemKey, traceHeader);
context.Items.Add(BaggageHeaderItemKey, baggageHeader);
context.Items.Add(TransactionContextItemKey, transactionContext);
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved

hub.ConfigureScope(scope =>
{
// At the point lots of stuff from the request are not yet filled
Expand Down
30 changes: 16 additions & 14 deletions src/Sentry.AspNetCore/SentryTracingMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,19 @@ public SentryTracingMiddleware(

try
{
var hub = _getHub();

// Attempt to start a transaction from the trace header if it exists
var traceHeader = context.TryGetSentryTraceHeader(_options);
// The trace gets continued in the SentryMiddleware.
context.Items.TryGetValue(SentryMiddleware.TransactionContextItemKey, out var transactionContextObject);
if (transactionContextObject is not TransactionContext transactionContext)
{
throw new KeyNotFoundException($"Failed to retrieve the '{nameof(SentryMiddleware.TransactionContextItemKey)}' from the HttpContext items.");
}

// It's important to try and set the transaction name to some value here so that it's available for use
// in sampling. At a later stage, we will try to get the transaction name again, to account for the
// other middlewares that may have ran after ours.
var transactionName = context.TryGetTransactionName() ?? string.Empty;

var transactionContext = traceHeader is not null
? new TransactionContext(transactionName, OperationName, traceHeader, TransactionNameSource.Route)
: new TransactionContext(transactionName, OperationName, TransactionNameSource.Route);
transactionContext.Name = context.TryGetTransactionName() ?? string.Empty;
transactionContext.Operation = OperationName;
transactionContext.NameSource = TransactionNameSource.Route;

var customSamplingContext = new Dictionary<string, object?>(4, StringComparer.Ordinal)
{
Expand All @@ -58,11 +58,14 @@ public SentryTracingMiddleware(
[SamplingExtensions.KeyForHttpContext] = context,
};

// Set the Dynamic Sampling Context from the baggage header, if it exists.
var baggageHeader = context.TryGetBaggageHeader(_options);
var traceHeader = context.Items.TryGetValue(SentryMiddleware.TraceHeaderItemKey, out var traceHeaderObject)
? traceHeaderObject as SentryTraceHeader : null;
var baggageHeader = context.Items.TryGetValue(SentryMiddleware.BaggageHeaderItemKey, out var baggageHeaderObject)
? baggageHeaderObject as BaggageHeader : null;

var dynamicSamplingContext = baggageHeader?.CreateDynamicSamplingContext();

if (traceHeader is { } && baggageHeader is null)
if (traceHeader is not null && baggageHeader is null)
{
// We received a sentry-trace header without a baggage header, which indicates the request
// originated from an older SDK that doesn't support dynamic sampling.
Expand All @@ -73,7 +76,7 @@ public SentryTracingMiddleware(
dynamicSamplingContext = DynamicSamplingContext.Empty;
}

var transaction = hub.StartTransaction(transactionContext, customSamplingContext, dynamicSamplingContext);
var transaction = _getHub().StartTransaction(transactionContext, customSamplingContext, dynamicSamplingContext);

_options.LogInfo(
"Started transaction with span ID '{0}' and trace ID '{1}'.",
Expand All @@ -95,7 +98,6 @@ public SentryTracingMiddleware(
public async Task InvokeAsync(HttpContext context)
{
var hub = _getHub();

if (!hub.IsEnabled)
{
await _next(context).ConfigureAwait(false);
Expand Down
12 changes: 6 additions & 6 deletions src/Sentry/BaggageHeader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Sentry;
/// </summary>
/// <seealso href="https://develop.sentry.dev/sdk/performance/dynamic-sampling-context"/>
/// <seealso href="https://www.w3.org/TR/baggage"/>
internal class BaggageHeader
public class BaggageHeader
jamescrosswell marked this conversation as resolved.
Show resolved Hide resolved
{
internal const string HttpHeaderName = "baggage";
internal const string SentryKeyPrefix = "sentry-";
Expand All @@ -19,14 +19,14 @@ internal class BaggageHeader
// "Uniqueness of keys between multiple list-members in a baggage-string is not guaranteed."
// "The order of duplicate entries SHOULD be preserved when mutating the list."

public IReadOnlyList<KeyValuePair<string, string>> Members { get; }
internal IReadOnlyList<KeyValuePair<string, string>> Members { get; }

private BaggageHeader(IEnumerable<KeyValuePair<string, string>> members) =>
Members = members.ToList();

// We can safely return a dictionary of Sentry members, as we are in control over the keys added.
// Just to be safe though, we'll group by key and only take the first of each one.
public IReadOnlyDictionary<string, string> GetSentryMembers() =>
internal IReadOnlyDictionary<string, string> GetSentryMembers() =>
Members
.Where(kvp => kvp.Key.StartsWith(SentryKeyPrefix))
.GroupBy(kvp => kvp.Key, kvp => kvp.Value)
Expand Down Expand Up @@ -59,7 +59,7 @@ public override string ToString()
/// <returns>
/// An object representing the members baggage header, or <c>null</c> if there are no members parsed.
/// </returns>
public static BaggageHeader? TryParse(string baggage, bool onlySentry = false)
internal static BaggageHeader? TryParse(string baggage, bool onlySentry = false)
{
// Example from W3C baggage spec:
// "key1=value1;property1;property2, key2 = value2, key3=value3; propertyKey=propertyValue"
Expand Down Expand Up @@ -107,7 +107,7 @@ public override string ToString()
return members.Count == 0 ? null : new BaggageHeader(members);
}

public static BaggageHeader Create(
internal static BaggageHeader Create(
IEnumerable<KeyValuePair<string, string>> items,
bool useSentryPrefix = false)
{
Expand All @@ -121,7 +121,7 @@ public static BaggageHeader Create(
return new BaggageHeader(members);
}

public static BaggageHeader Merge(IEnumerable<BaggageHeader> baggageHeaders) =>
internal static BaggageHeader Merge(IEnumerable<BaggageHeader> baggageHeaders) =>
new(baggageHeaders.SelectMany(x => x.Members));

private static bool IsValidKey(string? key)
Expand Down
30 changes: 26 additions & 4 deletions src/Sentry/DynamicSamplingContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ private DynamicSamplingContext(
SentryId traceId,
string publicKey,
bool? sampled,
double sampleRate,
double? sampleRate = null,
string? release = null,
string? environment = null,
string? userSegment = null,
Expand All @@ -49,15 +49,19 @@ private DynamicSamplingContext(
{
["trace_id"] = traceId.ToString(),
["public_key"] = publicKey,
["sample_rate"] = sampleRate.ToString(CultureInfo.InvariantCulture)
};

// Set optional values
if (sampled.HasValue)
{
items.Add("sampled", sampled.Value ? "true" : "false");
}

// Set optional values
if (sampleRate is not null)
{
items.Add("sample_rate", sampleRate.Value.ToString(CultureInfo.InvariantCulture));
}

if (!string.IsNullOrWhiteSpace(release))
{
items.Add("release", release);
Expand Down Expand Up @@ -119,7 +123,7 @@ private DynamicSamplingContext(
public static DynamicSamplingContext CreateFromTransaction(TransactionTracer transaction, SentryOptions options)
{
// These should already be set on the transaction.
var publicKey = Dsn.Parse(options.Dsn!).PublicKey;
var publicKey = options.ParsedDsn.PublicKey;
var traceId = transaction.TraceId;
var sampled = transaction.IsSampled;
var sampleRate = transaction.SampleRate!.Value;
Expand All @@ -140,6 +144,21 @@ public static DynamicSamplingContext CreateFromTransaction(TransactionTracer tra
userSegment,
transactionName);
}

public static DynamicSamplingContext CreateFromPropagationContext(SentryPropagationContext propagationContext, SentryOptions options)
{
var publicKey = options.ParsedDsn.PublicKey;
var traceId = propagationContext.TraceId;
var release = options.SettingLocator.GetRelease();
var environment = options.SettingLocator.GetEnvironment();

return new DynamicSamplingContext(
traceId,
publicKey,
null,
release: release,
environment:environment);
}
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved
}

internal static class DynamicSamplingContextExtensions
Expand All @@ -149,4 +168,7 @@ internal static class DynamicSamplingContextExtensions

public static DynamicSamplingContext CreateDynamicSamplingContext(this TransactionTracer transaction, SentryOptions options)
=> DynamicSamplingContext.CreateFromTransaction(transaction, options);

public static DynamicSamplingContext CreateDynamicSamplingContext(this SentryPropagationContext propagationContext, SentryOptions options)
=> DynamicSamplingContext.CreateFromPropagationContext(propagationContext, options);
}
23 changes: 23 additions & 0 deletions src/Sentry/Extensibility/DisabledHub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,29 @@ public void BindException(Exception exception, ISpan span)
/// </summary>
public SentryTraceHeader? GetTraceHeader() => null;

/// <summary>
/// Returns null.
/// </summary>
public SentryTraceHeader? GetTraceParent() => null;

/// <summary>
/// Returns null.
/// </summary>
public BaggageHeader? GetBaggage() => null;

/// <summary>
/// Returns null.
/// </summary>
public TransactionContext ContinueTrace(
SentryTraceHeader? traceHeader,
BaggageHeader? baggageHeader,
string? name = null,
string? operation = null)
{
// Transactions from DisabledHub are always sampled out
return new TransactionContext(string.Empty, string.Empty, false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could do the same thing we've done for the DisabledHub.Instance here, to avoid creating a new object every time this method is called.

Not sure where to put that... maybe TransactionContext.Disabled or TransactionContext.DisabledInstance?

}

/// <summary>
/// No-Op.
/// </summary>
Expand Down
18 changes: 18 additions & 0 deletions src/Sentry/Extensibility/HubAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,24 @@ public void BindException(Exception exception, ISpan span) =>
public SentryTraceHeader? GetTraceHeader()
=> SentrySdk.GetTraceHeader();

/// <summary>
/// Forwards the call to <see cref="SentrySdk"/>.
/// </summary>
[DebuggerStepThrough]
public BaggageHeader? GetBaggage()
=> SentrySdk.GetBaggage();

/// <summary>
/// Forwards the call to <see cref="SentrySdk"/>.
/// </summary>
[DebuggerStepThrough]
public TransactionContext ContinueTrace(
SentryTraceHeader? traceHeader,
BaggageHeader? baggageHeader,
string? name = null,
string? operation = null)
=> SentrySdk.ContinueTrace(traceHeader, baggageHeader, name, operation);

/// <summary>
/// Forwards the call to <see cref="SentrySdk"/>.
/// </summary>
Expand Down
19 changes: 18 additions & 1 deletion src/Sentry/IHub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,27 @@ ITransaction StartTransaction(
ISpan? GetSpan();

/// <summary>
/// Gets the Sentry trace header for the last active span.
/// Gets the Sentry trace header that allows tracing across services
/// </summary>
SentryTraceHeader? GetTraceHeader();

/// <summary>
/// Gets the Sentry baggage header that allows tracing across services
/// </summary>
BaggageHeader? GetBaggage();

/// <summary>
/// Continues a trace based on HTTP header values.
/// </summary>
/// <remarks>
/// If no "sentry-trace" header is provided a random trace ID and span ID is created.
/// </remarks>
TransactionContext ContinueTrace(
SentryTraceHeader? traceHeader,
BaggageHeader? baggageHeader,
string? name = null,
string? operation = null);

/// <summary>
/// Starts a new session.
/// </summary>
Expand Down
Loading
Loading