Performance API follow up

[bruno-garcia]

I've tried to add some docs, and in the process a few things came up.

Docs PR: getsentry/sentry-docs#2790

Notes:

• StartTransaction vs CreateTransaction
  Sentry's SDKs use startTransaction but .NET has CreateTransaction

Java requires only "name" and .NET requires "name" and "op".
Code snippets in Java set an Op right after creating it, should it be in the Start method instead?

Java has Sentry.getSpan(); to get the current "span bound to the Hub". How do we do this in .NET?
Is this in JavaScript? If not, how do we do it there?

// C#:
var innerSpan = span.StartChild("task");

// Java
Span innerSpan = span.startChild("task", item.getName());

How do I set the span name in C#? Only operation in the method signature.

var transaction = SentrySdk.CreateTransaction("ProcessOrderBatch()", "batch");
try 
{
    procesOrderBatch();
    transaction.Finish();
}
catch (Exception e)
{
    // How do I set the exception? Or relate to an error event?
    // Java: transaction.setThrowable(e);
    transaction.Finish(SpanStatus.InternalError);

	// this
    SentrySdk.CaptureException(e); // How is the transaction and error linked?
	// or
	throw; // I expect the request is logged upstream so the reference to the Exception must have been given to the transaction earlier in this catch block
}

In Java we set the exception to the transaction, which is kept in a WeakRef so we can corelate the error event to the transaction.
How do we do this in C#?

Java has:

ISpan span = Sentry.getSpan();
if (span == null) {
    span = Sentry.startTransaction("task");
} else {
    span = span.startChild();
}

How do we do this? Should we change Java to something else. I believe Go doesn't distinct between StartTran or StartSpan (thinking in the future with single span ingestion, does it matter?)

[Tyrrrz]

@bruno-garcia

Java requires only "name" and .NET requires "name" and "op".

What should be the default value for op be? Also, in some SDKs the entire object is mutable, should we do the same thing here as well?

Java has Sentry.getSpan(); to get the current "span bound to the Hub". How do we do this in .NET?

What does a "current span" mean? Last created transaction?

How do I set the span name in C#? Only operation in the method signature.

Do spans actually have names? From what I know, they don't.

// How do I set the exception? Or relate to an error event?
// Java: transaction.setThrowable(e);

What does that do? The docs don't seem to cover that.

Java has:

Do you mean Sentry.getSpan()? I think this question was already asked above. The docs don't seem to mention it (although they mention setSpan(): https://develop.sentry.dev/sdk/unified-api/tracing/#static-api-changes

[Tyrrrz]

@bruno-garcia ping

[bruno-garcia]

What should be the default value for op be? Also, in some SDKs the entire object is mutable, should we do the same thing here as well?

I don't know. And I rather it's not mutable unless it must be. I date to say folks might not have attempted to make it immutable on other SDKs yet.

What needs to be mutable? Btw if it's mutable, it needs to be thread-safe. THat's not a concern on some other SDKs (single threaded like node, php and ruby).

What does a "current span" mean? Last created transaction?

I believe it's the last created span in the tree. Is the definition written somewhere? We need to get it defined in the docs so others can implement this. @HazAT or @marandaneto might chime in.

Do spans actually have names? From what I know, they don't.

If we have name in Java then we should probably get rid of it. @maciejwalkowiak maybe knows.

What does that do? The docs don't seem to cover that.

Maciej and I came up with that in a way to bind an exception to the span which it was running under. I believe other SDKs don't have that at all. We need to document this. @maciejwalkowiak could you please document that in the Java docs? Basically we need to related the span and the exception we are about to rethrow for the error tracking integration to capture upstream.

void Framework() {
try {
 next(); // Calls the app code that throws
} catch (e) {
 // here we log with all the context we have
 var e = SentryEventWithHttpContextData();
 Sentry.Capture(e)
}
}

void SentryHttpLibraryIntegration() {
using (var span = sentry.StartSpan("HTTP") {
try {
// Here we intercept an outgoing HTTP request
} catch (e) {

span.MarkAsFailed(error);
// How do I coorelate this span here with the event that will be sent from the rethrown exception?
span.StoreWeakReferenceOfException(e);

throw; // not our problem to deal with this.
}
}
}

[maciejwalkowiak]

If we have name in Java then we should probably get rid of it. @maciejwalkowiak maybe knows.

Spans don't have a name but have op (operation) and description.

What does a "current span" mean? Last created transaction?

Current span is the span that has been started but has not been finished yet. If there is no current ongoing span, getSpan should return null.

Regarding linking events with spans:

To mark event as one that happened during the particular span, event trace context must be set to the same values as the span. This has to be tackled in two ways:

1. Set trace context on the event that is actually triggered within a span.

In Java it's done in SentryClient: https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/SentryClient.java#L438

2. Connect event created AFTER the span is already finished with a span

This is where we needed a weak reference.

Both Transaction and Span has a transient field: throwable, which is added to weak hashmap when transaction/span is finished https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/Span.java#L65

Then when an event is created from the caught exceptions, we look up this span/transaction and set it on the event: https://github.com/getsentry/sentry-java/blob/main/sentry-spring/src/main/java/io/sentry/spring/SentryExceptionResolver.java#L55

[Tyrrrz]

Notes from meeting

• Is operation required and mutable?
• Change createTransaction to startTransaction
• Add startChild(op, description) because description is often used
• Event with errors must link to a span if it exists (setThrowable)
• Make fields mutable (i.e. transaction name, operation, etc)
• Integrate sampling context
• Integrate trace header
• Add tracing context
• Do we need DisabledTransaction and DisabledSpan? seeing as they wrap IHub
• Expose EventId in Transaction

[Tyrrrz]

@bruno-garcia do we want to rename Transaction to SentryTransaction as well? What's the convention around these things?

[Tyrrrz]

@bruno-garcia one more nitty question:

SentryOptions.TraceSampleRate or SentryOptions.TracesSampleRate? .NET has the former, Java has the latter. (crossed out, the guidelines also suggest SentryOptions.TracesSampleRate)

Also, in .NET SentryOptions.SampleRate is a nullabe float, while SentryOptions.TraceSampleRate is a non-nullable double. Java has both as non-nullable doubles. I think we should change SentryOptions.SampleRate to match, since it's a major bump. What do you think?

[Tyrrrz]

@bruno-garcia in regards to sampling, Java has TracesSamplerCallback, which is a delegate. I see that the convention in the .NET SDK seems to be to use interfaces instead. Which one should we do? The guidelines suggest callback.

[Tyrrrz]

@bruno-garcia in .NET we have Transaction.Name, Scope.Transaction, and Scope.TransactionName (legacy), Java has Transaction.get/setTransaction and Scope.getTransactionName, Scope.getTransaction, and Scope.setTransaction. Should we rename?

[bruno-garcia]

@bruno-garcia in regards to sampling, Java has TracesSamplerCallback, which is a delegate. I see that the convention in the .NET SDK seems to be to use interfaces instead. Which one should we do? The guidelines suggest callback.

A callback is fine (we have BeforeSend). The interface plays well with IoC and ASP.NET Core so you can register your samples in the container but lets stay aligned here and we can add some interface straight to aspnetcore later if we decide to.

@bruno-garcia in .NET we have Transaction.Name, Scope.Transaction, and Scope.TransactionName (legacy), Java has Transaction.get/setTransaction and Scope.getTransactionName, Scope.getTransaction, and Scope.setTransaction. Should we rename?

Transaction.Transaction ? That would be the equivalent to Transaction.setTransaction in Java, right? I think both are not ideal. It's really messy this thing with Transaction Name string for errors and the new Transaction class. I don't have a solution for this, I saw some comments from @rhcarvalho today and @marandaneto, @maciejwalkowiak maybe they have some solution.

[maciejwalkowiak]

Java has Transaction.get/setTransaction and Scope.getTransactionName, Scope.getTransaction, and Scope.setTransaction. Should we rename?

SentryTransaction#transaction ideally would be "name", but this is the json property that's expected on the backend. If we want custom property we would need to write custom json serializer. Anyway, this property is hidden from the user. From the user point of view, he passes "name" to SentryTransaction constructor.

Regarding Scope#setTransactionName - it's taken into account only if there is an ongoing transaction - then we call SentryTransaction#setName. If there is no transaction set on scope, calling setTransactionName is no-op. I've lost track a bit - is this behavior incorrect?

[Tyrrrz]

@maciejwalkowiak
I believe it shouldn't be a complete no-op, as the value passed to setTransactionName should be used by events. /cc @rhcarvalho
Relevant: getsentry/develop#246

[Tyrrrz]

Closing this, as the only remaining issue is tracked in #738