rfc(decision): Revamping the SDK Performance API

[AbhiPrasad]

Rendered RFC

closes #28

This RFC proposes to revamp the performance API in the SDKs. The new API aims to accomplish the following:

• Align both the internal schemas and top level public API with OpenTelemetry and their SDKs.
• De-emphasize the concept of transactions from users using a Sentry performance monitoring SDK.
• Optimize for making sure performance data is always sent to Sentry.
• Open the SDK up for future work where we support batch span ingestion instead of relying on a transaction solely.

The RFC proposal comes in two steps.

1. Introduce three new top level methods, Sentry.startActiveSpan and Sentry.startSpan and their language specific variants as well as Sentry.setMeasurement.

2. Introduce a new span schema that is aligned with OpenTelemetry. Out of scope for this RFC, will be addressed later.

[marandaneto]

I'm wondering if this solves (or not) the use case that you have an active span (span A), and spawn multiple threads to download something, and each of those threads will start a new span (span B and C), but then the relationship of the spans are messed up because they "race each other", for example, span B and C should be a child of span A, but what I see is that span C is a child of span B.

[AbhiPrasad]

I guess the caveat here is that the hub/scope needs to be properly forked and correctly follow the execution context.

I guess in single scope instances, this logic breaks down re: parent-child relationships. Let me amend the RFC to reference this.

[marandaneto]

Yeah, Mobile is mostly since scope instance.

[krystofwoldrich]

Defining a root span for mobile/single-scope SDKs sounds good. It doesn't solve the parent-child relationships, but keeps the behaviour the same as with Transactions and Span.

[marandaneto]

I believe is important to keep description, how would I set the SQL statement of a DB query if it has to be low cardinality? One can say you can do it under the data field, so the name then would be similar to the former operation? some operations just don't have a unique name, is the grouping of transactions still per name?

[AbhiPrasad]

In the OpenTelemetry conventions, it's recommended db spans be named <db.operation> <db.name>.<db.sql.table> - like SELECT bestDb.orders, but the only thing they require is that something is low cardinality, so the db query also works here. In most cases, we can just add whatever we were setting in span.description to span.name (as long as it's low cardinality).

Grouping of transactions/spans would still be by name, but there is more freedom about what that should be. The product instead relies on span attributes to make this work.

[marandaneto]

I see, DB queries are high cardinality though, at least for bigger databases.
Most of the time interceptors/hooks just give you a String, so to build SELECT bestDb.orders requires parsing the String on the client which is also not ideal.
Alright, this is more an implementation detail anyway, thanks.

[philipphofmann]

I'm OK removing op and adding a new low cardinality format of OpenTelemetry as you described @AbhiPrasad, but I think the description we currently have for db queries, HTTP requests, and such is highly useful. Where would that end up now?

[marandaneto]

@philipphofmann the span attributes, aka span.data

[marandaneto]

Can we keep the data that is gonna be deprecated and forwarded to attributes for compatibility?
eg
span.setData(x, y) just calls span.setAttribute(x, y) internally.

[AbhiPrasad]

Yup this is a good idea! I will also put more details into migrating between the different span schemas.

[philipphofmann]

Excellent that you try to address some critical issues of our performance API @AbhiPrasad 👏. I gave this a first quick pass, but I'll have to take some more time after my vacation next week and think about how the new API will play out on mobile.

[philipphofmann]

I guess that should be step 2.

Suggested change

	1. Introduce a new span schema that is aligned with OpenTelemetry.
	2. Introduce a new span schema that is aligned with OpenTelemetry.

[philipphofmann]

Nice summary of the biggest problems 👍

[philipphofmann]

Is this already an official decision that Sentry decided to align more with OpenTelemetry? If yes, please link the decision or elaborate somewhere in the document on why we want to achieve this, as I think it might not be 100% clear to everyone.

[marandaneto]

I'd be interested in that as well, what are the pro x cons of moving closer to opentelemetry rather than our own model?
I know a few of them but it'd be nice to be written down.

[philipphofmann]

I guess that should not be commented out

Suggested change

	// declare function startActiveSpanAsync<T>(
	// spanContext: SpanContext,
	// callback: (span: Span) => Promise<T>,
	// ): Promise<T>;
	declare function startActiveSpanAsync<T>(
	spanContext: SpanContext,
	callback: (span: Span) => Promise<T>,
	): Promise<T>;

[philipphofmann]

I was a bit confused by the difference of startActiveSpan and startSpan when reading it for the first time. Even after reading the explanation, I'm still unhappy with the name. The name states to start a span, but the method takes a lamba and starts and finishes the span. I think the name should reflect that, so it should be something like executeSpan, createSpan, etc.

Furthermore, I'd like to have the flexibility of finishing the span manually and achieving the same behavior you described with startActiveSpan or will you still be able to do startSpan(bindtoScope: true)? If yes, then it's a bit confusing to me to achieve the same behavior with either startActiveSpan plus a lambda and startSpan(bindtoScope: true).

I'm sorry I didn't come up with proposals for solutions yet. That is not an easy problem to solve and I have to take some time to think about it.

[marandaneto]

@philipphofmann If I got it right, startActiveSpan is the equivalent of bindtoScope: true, and startSpan the equivalent of bindtoScope: false, hence the active naming, because it is the one active in the scope.

[krystofwoldrich]

Active for active in the scope in the scope make sense, but since we try to shield users from the internal scope and hub.

startActiveSpan looks to me as active for the automatic finish.
startSpan as a manual variant of the startAtiveSpan

[philipphofmann]

No tags for spans?

[marandaneto]

Today spans have tags, but they are not indexed, so they can just be span attributes.
But I'd love to see indexed fields from spans as well, I believe this is important.

[philipphofmann]

I'm OK removing op and adding a new low cardinality format of OpenTelemetry as you described @AbhiPrasad, but I think the description we currently have for db queries, HTTP requests, and such is highly useful. Where would that end up now?

[philipphofmann]

Can we also keep origin https://develop.sentry.dev/sdk/performance/trace-origin/? It's handy to know what precisely created a span.

[marandaneto]

origin can also be part of the span attributes if needed, but good point.

[iker-barriocanal]

What's the plan with transaction-specific information that doesn't belong to dynamic sampling or tracing? Is it going to be duplicated in every span? Tags, for example.

[iker-barriocanal]

Is this a good time to structure the data further and define what Object looks like for each field? The more we structure the data, the better product experience we can provide on the server side.

[cleptric]

Sentry.startActiveSpan should be the default, hence called Sentry.startSpan.
Introducing more top-level functions (even undocumented) increases the API surface unnecessarily and adds more confusion.

[HazAT]

What if we turn it around - instead of having startActiveSpan we just have startSpan that does by default what startActiveSpan does (meaning putting it on the scope) - I suppose this is what people would expect 99% of the time. We could add an argument that prevents setting it on the scope if someone sets it.

[marandaneto]

That's what we do on Mobile, startTransaction with a bindToScope param, but it's bindToScope=false by default.

If this flag is enabled by default, because of single Hub/Scope on Mobile, might be overwriting each other transactions as a side-effect, not sure if it's a good idea, at least on the Mobile world, maybe keeping bindToScope=false by default makes more sense in this case.

The other option is that if it's enabled by default, but there's a living transaction in the scope, you just create a span, but maybe this is not the desired behavior and we'd need a way to opt-out, in this case, people would need to force bindToScope=false.

Ergonomics isn't great again because they will not know if there's a living transaction in the scope.

[cleptric]

Any use-cases come to mind why this function is necessary?

[marandaneto]

Mobile has single Hub/Scope and this is needed in case you want your own transaction instead of the auto-instrumented ones, measuring something side by side.
For example, a auto-instrumented transaction during screen loading, but also a service doing the X job, that has nothing to do with the screen loading transaction.

[cleptric]

What are we going to do with the extra stuff?

[marandaneto]

#101 (comment)

[HazAT]

[AbhiPrasad]

After a lot of deliberation finally pushed updates to this RFC.

We've moved the span schema changes out of scope of this RFC as this requires further discussion in ingest/product. As a result the RFC now only focuses on introducing the new top level methods for the performance API.

The RFC has also been amended to give more freedom to SDK authors to decide what methods to best introduce, although the recommendation is to use startActiveSpan and startSpan. This was done because certain languages/runtimes might have more idiomatic patterns for span creation, and we should prefer that over forcing a single unified API everywhere. We see this in OpenTelemetry where each otel SDK has it's own set of span creation methods.

// golang
ctx, span := tracer.Start

// .NET
using var myActivity = MyActivitySource.StartActivity

# ruby
MyAppTracer.in_span("do_work") do |span|
  # do some work that the 'do_work' span tracks!
end

# Java
Span childSpan = tracer.spanBuilder("child")
    .setParent(Context.current().with(parentSpan))
    .startSpan();