-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
Support performance #633
Support performance #633
Changes from 33 commits
f31adac
79d2397
d7c9974
51783e8
c99635d
7ad5ab5
96c443e
98e567d
e0eb730
748670c
dfdec31
fa64053
8338381
8fc5c2b
6a05871
6c0b928
5a47bd9
e67597e
52ec0a5
15d7001
3a705f0
c4f9371
00ef2b5
b0487cf
0a713f3
c314a66
9e2b7e1
d0c5358
a4e01d8
d960d7f
a0fb93e
6c1b413
258ed5c
f2e5a8b
41bcb99
da7ee94
1371006
bafe82a
7abdb4f
a1e48f8
fd13150
134d0ec
6b92788
b6f3ee2
0e91066
ea06cfb
2cb49ef
e908cee
e8aa3cc
71c87ca
0c512b1
fbd6f17
13b88db
eb37814
61085a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,7 +87,9 @@ public static void Populate(this Scope scope, HttpContext context, SentryAspNetC | |
scope.SetTag("route.area", area); | ||
} | ||
|
||
scope.Transaction = area == null ? $"{controller}.{action}" : $"{area}.{controller}.{action}"; | ||
scope.TransactionName = area == null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have this in some internal static method to make sure changing its stays aligned with the one on the middleware? |
||
? $"{controller}.{action}" | ||
: $"{area}.{controller}.{action}"; | ||
} | ||
} | ||
catch(Exception e) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
using System; | ||
using System.Threading.Tasks; | ||
using Sentry.Protocol; | ||
|
||
namespace Sentry.Extensibility | ||
{ | ||
|
@@ -51,6 +52,17 @@ public void WithScope(Action<Scope> scopeCallback) | |
{ | ||
} | ||
|
||
/// <summary> | ||
/// Returns a dummy transaction. | ||
/// </summary> | ||
public Transaction CreateTransaction(string name, string operation) => | ||
new Transaction(this, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should have a singleton NoOp instance here instead. |
||
|
||
/// <summary> | ||
/// Returns null. | ||
/// </summary> | ||
public SentryTraceHeader? GetSentryTrace() => null; | ||
|
||
/// <summary> | ||
/// No-Op. | ||
/// </summary> | ||
|
@@ -63,6 +75,13 @@ public void BindClient(ISentryClient client) | |
/// </summary> | ||
public SentryId CaptureEvent(SentryEvent evt, Scope? scope = null) => SentryId.Empty; | ||
|
||
/// <summary> | ||
/// No-Op. | ||
/// </summary> | ||
public void CaptureTransaction(Transaction transaction) | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// No-Op. | ||
/// </summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
namespace Sentry | ||
{ | ||
/// <summary> | ||
/// Trace sampler. | ||
/// </summary> | ||
public interface ISentryTraceSampler | ||
{ | ||
/// <summary> | ||
/// Gets the sample rate based on context. | ||
/// </summary> | ||
double GetSampleRate(TraceSamplingContext context); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Threading.Tasks; | ||
using Sentry.Extensibility; | ||
using Sentry.Integrations; | ||
using Sentry.Protocol; | ||
|
||
namespace Sentry.Internal | ||
{ | ||
|
@@ -99,6 +101,39 @@ public void WithScope(Action<Scope> scopeCallback) | |
|
||
public void BindClient(ISentryClient client) => ScopeManager.BindClient(client); | ||
|
||
public Transaction CreateTransaction(string name, string operation) | ||
{ | ||
var trans = new Transaction(this, _options) | ||
{ | ||
Name = name, | ||
Operation = operation | ||
}; | ||
|
||
var nameAndVersion = MainSentryEventProcessor.NameAndVersion; | ||
var protocolPackageName = MainSentryEventProcessor.ProtocolPackageName; | ||
|
||
if (trans.Sdk.Version == null && trans.Sdk.Name == null) | ||
{ | ||
trans.Sdk.Name = Constants.SdkName; | ||
trans.Sdk.Version = nameAndVersion.Version; | ||
} | ||
|
||
if (nameAndVersion.Version != null) | ||
{ | ||
trans.Sdk.AddPackage(protocolPackageName, nameAndVersion.Version); | ||
} | ||
|
||
ConfigureScope(scope => scope.Transaction = trans); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe by default we won't do this. The snippets I saw from JS require the caller to decide whether the transaction should be bound to the scope or not. For now this looks good and lets try like this. |
||
|
||
return trans; | ||
} | ||
|
||
public SentryTraceHeader? GetSentryTrace() | ||
{ | ||
var (currentScope, _) = ScopeManager.GetCurrent(); | ||
return currentScope.Transaction?.GetTraceHeader(); | ||
} | ||
|
||
public SentryId CaptureEvent(SentryEvent evt, Scope? scope = null) | ||
{ | ||
try | ||
|
@@ -128,6 +163,18 @@ public void CaptureUserFeedback(UserFeedback userFeedback) | |
} | ||
} | ||
|
||
public void CaptureTransaction(Transaction transaction) | ||
{ | ||
try | ||
{ | ||
_ownedClient.CaptureTransaction(transaction); | ||
} | ||
catch (Exception e) | ||
{ | ||
_options.DiagnosticLogger?.LogError("Failure to capture transaction: {0}", e, transaction.SpanId); | ||
} | ||
} | ||
|
||
public async Task FlushAsync(TimeSpan timeout) | ||
{ | ||
try | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,10 +31,10 @@ internal class MainSentryEventProcessor : ISentryEventProcessor | |
: null; | ||
}); | ||
|
||
private static readonly SdkVersion NameAndVersion | ||
internal static readonly SdkVersion NameAndVersion | ||
= typeof(ISentryClient).Assembly.GetNameAndVersion(); | ||
|
||
private static readonly string ProtocolPackageName = "nuget:" + NameAndVersion.Name; | ||
internal static readonly string ProtocolPackageName = "nuget:" + NameAndVersion.Name; | ||
|
||
private readonly SentryOptions _options; | ||
internal Func<ISentryStackTraceFactory> SentryStackTraceFactoryAccessor { get; } | ||
|
@@ -96,20 +96,17 @@ public SentryEvent Process(SentryEvent @event) | |
|
||
@event.Platform = Protocol.Constants.Platform; | ||
|
||
if (@event.Sdk != null) | ||
// SDK Name/Version might have be already set by an outer package | ||
// e.g: ASP.NET Core can set itself as the SDK | ||
if (@event.Sdk.Version == null && @event.Sdk.Name == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are transactions going through this at the end? Doesn't seem like it since this take SentryEvent. So transactions will always be reported as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah... The SDK is set separately when creating the transaction right now. Transactions also don't get all the other default metadata either, which is populated by event processors. |
||
{ | ||
// SDK Name/Version might have be already set by an outer package | ||
// e.g: ASP.NET Core can set itself as the SDK | ||
if (@event.Sdk.Version == null && @event.Sdk.Name == null) | ||
{ | ||
@event.Sdk.Name = Constants.SdkName; | ||
@event.Sdk.Version = NameAndVersion.Version; | ||
} | ||
@event.Sdk.Name = Constants.SdkName; | ||
@event.Sdk.Version = NameAndVersion.Version; | ||
} | ||
|
||
if (NameAndVersion.Version != null) | ||
{ | ||
@event.Sdk.AddPackage(ProtocolPackageName, NameAndVersion.Version); | ||
} | ||
if (NameAndVersion.Version != null) | ||
{ | ||
@event.Sdk.AddPackage(ProtocolPackageName, NameAndVersion.Version); | ||
} | ||
|
||
// Report local user if opt-in PII, no user was already set to event and feature not opted-out: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's on the line below (it got some ` on another PR)