Use the existing scope for EFCore

[Belphemur]

Instead of always tracing on the transaction, checking if there is a span already would be better to properly link the span to it.

[mattjohnsonpint]

Interesting. And thanks for the PR!

I think you are probably correct, but would you please elaborate on the use case? What problem does it solve for you? Thanks.

[mattjohnsonpint]

Also, a unit test would be super helpful.

[Belphemur]

@mattjohnsonpint Here is a screenshot of the problem I encounter

Basically, I start already my own span that are under the main transaction. In those span, there are some query being run.

So what I'm trying to achieve is what the http.client (in blue) already does, find the closest span and attach to it. My issue is with the red, with the span ShowRefresher - save-in-db. As you can see the database call aren't linked to the span but to the transaction directly.

Edit: Unit test created for this use case.

[mattjohnsonpint]

Looking over this closer, it seems that this change would make both branches of this function do the same thing.

private static ISpan? GetParent(SentryEFSpanType type, Scope scope)
{
    if (type == SentryEFSpanType.QueryExecution)
    {
        return scope.GetSpan();  // <-------- GetSpan does the same as below
    }

    return scope.Transaction?.GetLastActiveSpan() ?? scope.Transaction;
}

It looks like @lucas-zimerman added the branch for QueryExectution types in #1368. So this would undo that. @lucas-zimerman, can you give us some insight here? Thanks.

[lucas-zimerman]

Looking over this closer, it seems that this change would make both branches of this function do the same thing.

private static ISpan? GetParent(SentryEFSpanType type, Scope scope)
{
    if (type == SentryEFSpanType.QueryExecution)
    {
        return scope.GetSpan();  // <-------- GetSpan does the same as below
    }

    return scope.Transaction?.GetLastActiveSpan() ?? scope.Transaction;
}

It looks like @lucas-zimerman added the branch for QueryExectution types in #1368. So this would undo that. @lucas-zimerman, can you give us some insight here? Thanks.

The goal was to have both Connection and query compile linked to the scope/root transaction where the query spans would be linked to the last opened.

It is more of an aesthetic change, you may get a result that may not be the desired one, as shown below a stack of connections connected to each other and not directly connected to the ShowRefresher

Might be too much but maybe add an option where users decide how to group connections? (to the root transaction or to the last opened span)

[Belphemur]

Wouldn't it be better to create a span from the latest span when creating the connection. Then remember that specific span for the duration of the db connection and use it to trace the db.compile and db.execute.

This would avoid having to use GetSpan for subsequent calls and have everything linked properly.

[Belphemur]

I was also thinking that the code isn't doing that right thing.

It should create a span from the latest span when creating the connection. Then remember that specific span for the duration of the db connection and use it to trace the db.compile and db.execute.

This way it's always properly link to the proper span.

Sent from Nine

---

From: Matt Johnson-Pint @.***> Sent: Thursday, May 26, 2022 17:27 To: getsentry/sentry-dotnet Cc: Antoine Aflalo; Author Subject: Re: [getsentry/sentry-dotnet] Use the existing scope for EFCore (PR #1671)

Looking over this closer, it seems that this change would make both branches of this function do the same thing.

private static ISpan? GetParent(SentryEFSpanType type, Scope scope) { if (type == SentryEFSpanType.QueryExecution) { return scope.GetSpan(); // <-------- GetSpan does the same as below }

return scope.Transaction?.GetLastActiveSpan() ?? scope.Transaction;

}

It looks like @lucas-zimerman added the branch for QueryExectution types in #1368. So this would undo that. @lucas-zimerman, can you give us some insight here? Thanks. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[mattjohnsonpint]

@Belphemur - What you described in your last comments would make a lot of sense. If you want to either update this PR, or close and open a new one, that would be greatly appreciated.

See also issue #2023 raised by @CrazyBaran, which appears to be asking for the same thing.

[CrazyBaran]

Hello guys, I am simple IT data enthusiast, the problem which we had is that we like to present 3D data on 2D chart, there is always lost of one of dimension. We need to answer the question, what is more important. db.connection or stack of execution of db.compile and db.query. In my opinion more important is the second one. Some heretic idea would be to add additional dimension into this chart, but I am understand that it would be crazy hard to achieve. Take a look into picture which I attach. Simple lines would show what was real span of execution, but I would rather see such line over db.connection. We still had issue with this bastards span (take a look into NullDataBrickService span, which show up in db.connection span. From my understanding it was run under span DataBricksRunAppService.CreateAsync, but this confusing magic move it into db.connection. In my opinion it is wrong. This issue give us long living db.connection very hard to analyze, I know that in basic dotnet usage nobody do this, but I work in finance and it is very common pattern. This information give nothing other than confusion. Why if I open connection and then work over it, all of my spans land inside? db.connection from developer point of view in dotnet is almost hidden from me, that's why we invent UnitOfWork. Please let me know openly if I did not understand something and I open for question, how I feel that. PS. Sorry for many edit i like to discuss my ideas with myself when I wrote such thing. PS2. It was even tested what happen when we use AddPooledDbContextFactory?

[CrazyBaran]

@mattjohnsonpint Maybe another idea. As documentation for distributed tracing say https://docs.sentry.io/product/sentry-basics/tracing/distributed-tracing/ Maybe we should consider all ef core spans as separate transaction inside of backend with the same traceId? then it would be visibile as on this picture.

[mattjohnsonpint]

@CrazyBaran - Thanks for your feedback. However, I believe the correct approach here is to attach db connections to the existing scope rather than the root of the transaction. Queries should continue to be attached to the connection itself.

With regard to your comment:

This issue give us long living db.connection very hard to analyze, I know that in basic dotnet usage nobody do this, but I work in finance and it is very common pattern.

As far as I am aware, long-lived db connections are an anti-pattern that should always be avoided. The accepted best practice is to open and close connections as needed, and rely on database connection pooling. I would expect Sentry to highlight such usage in exactly they way you say makes it hard to analyze, because that is the signal that you're leaving the connection open instead of pooling properly.

If that's a signal you want to discard, you can filter out the span when viewing the transaction in the UI. Though I strongly recommend that you switch to using short-lived connections with pooling.

[CrazyBaran]

@mattjohnsonpint what I understand in terms long living is connections which are created for each request and after that their are used to open db transaction. You can see how it looks like on my picture. You cannot really create unit of work pattern with transaction if we not open connection for example if we like rollback on some errors.

[mattjohnsonpint]

Closed in favor of #2071. Thanks.