Feature request: An option to have every LINQ statement default to using the current class and method name as a .TagWith value

[m60freeman]

As a DBA, I often find SQL in the SQL Server plan cache that is problematic in some way and ask the developers to modify it. Their response is inevitably that they cannot find the LINQ statement(s) that cause that code to be generated, often because there are so many similar possibilities or they are built up by many nested calls.

The .TagWith feature added in 2.2 is a step forward, but our developers are unlikely to be given the time it would take to manually add a .TagWith string to every LINQ statement individually in a very large code base.

I'd like to see an option to have every LINQ statement default to using the current class and method name as a .TagWith value. That way we could instantly get comments in the generated SQL that would let the developers instantly find the LINQ statements that result in any generated SQL statement I send them.

[ajcvickers]

@m60freeman This is something that we've discussed many times in the past. Do you have any suggestions as to how it might be implemented in an efficient and robust manner?

(See #14176, which is somewhat related.)

[m60freeman]

@ajcvickers My suggestion goes further than #14176 (although that would be a convenience), in that if implemented, mine would not require any modification of the user's code to manually add .TagWith at all. This would be a great advantage when desiring to instrument all LINQ statements in a very large code base (or in an environment that would require a code review of each class that was so modified).

I'm a DBA, not a .Net developer, so I hesitate to suggest how to implement this, but some type of dependency injection of a partial class using reflection that was triggered by an MSBuild defined constant might be an idea. At a previous job, one of the developers did something like that (without the defined constant) to implement the equivalent of my request (of course, he didn't have .TagWith() and wrote his own code to insert the comment into the generated TSQL).

As to whether it uses filename and line number or class and method doesn't much matter to me. I'm all for whatever makes it easier for a developer to find the LINQ statement that is generating the SQL code I am complaining about. :-)

[ajcvickers]

@m60freeman Problem is, we don't know how to do it without nasty hacks or fragile code.

[smitpatel]

We could provide an extensibility point to users which could be run on every query going through EF query compiler. So they could add tags (or tracking or anything else)

[ajcvickers]

@smitpatel Not clear to me how this would get information about the place the query was created or is being executed from.

[m60freeman]

Could Reflection get you the class and method from which the EF query compiler was getting invoked? It probably couldn't get you a file path and line number, but I really don't know.

[ajcvickers]

Discussed this again in triage, but again we don't know how to implement this in a robust and performant manner. If anyone can suggest such an implementation approach--that is, how we can trace where a query is coming from in code without any modification of the code at the call site--then we would consider re-opening this,

[jzabroski]

@m60freeman This is possible, thanks to the suggestions I made in #12669 (comment)

If you don't leak IQueryable<T> all over your application and use a Repository pattern to encapsulate your queries and return objects that fully encapsulate IQueryable<T> and support lazily evaluating the query, it should be trivial to implement this:

class PersonRepository : IRepository<Person> {
  DbContext _dbContext;
  PersonRepository(DbContext dbContext) {
    _dbContext = dbContext;
  }
  private static string GetTypeAndMethodName([CallerMemberName] string callerName = null)
  {
    return $"{typeof(MyClass).Name}.{callerName}";
  }
  IQueryable<Person> All() {
    return DbSet<Person>().TagWith($"{GetTypeAndMethodName()}");
  }
}

[m60freeman]

@ajcvickers Does the comment from @jzabroski help point a way forward for this?

[jzabroski]

I think the problem is that tags (really, TagExpressions are added to SelectExpression statements right now, as opposed to any Expression).

Here are the places where the implementation is referenced:

1. https://github.com/aspnet/EntityFrameworkCore/blob/5643bb6dcacaa294197e6649caac6a7a410434f5/src/EFCore.Relational/Query/Expressions/SelectExpression.cs
2. https://github.com/aspnet/EntityFrameworkCore/blob/dc51ab4aeef003175e64c41fe187ed5c0f153010/test/EFCore.Specification.Tests/Query/QueryTaggingTestBase.cs
3. https://github.com/aspnet/EntityFrameworkCore/blob/779d43731773d59ecd5f899a6330105879263cf3/test/EFCore.SqlServer.FunctionalTests/Query/QueryTaggingSqlServerTest.cs
4. https://github.com/aspnet/EntityFrameworkCore/blob/1fef013c122da8e18c121f7713337c0449d2baee/src/EFCore/Query/ResultOperators/Internal/TagExpressionNode.cs
5. https://github.com/aspnet/EntityFrameworkCore/blob/779d43731773d59ecd5f899a6330105879263cf3/src/EFCore/Query/ResultOperators/Internal/TagResultOperator.cs
6. https://github.com/aspnet/EntityFrameworkCore/blob/91fd469887e03b5f9c6f1775bcf1dabb5d667e7e/src/EFCore/Query/Internal/MethodInfoBasedNodeTypeRegistryFactory.cs
7. https://github.com/aspnet/EntityFrameworkCore/blob/43f040f510450cb4ba25e4690b0ddac88e1019ee/src/EFCore/EntityFrameworkQueryableExtensions.cs

I left a code review on the tags commit by @smitpatel and mentioned @divega , as I don't particularly understand why there is not a good way to implement this. It is possible I am misunderstanding what "good way" means or some subtlety, and I am not trying to put pressure on @ajcvickers to do something he believes is a bad idea - I just want to understand why and what design goals the EF team has in mind for a good design.

Off the top of my head, the one potential problem is that by putting the tag at the start of the expression, it's possible there could be multiple repeated tags, and so in the general case (leaking IQueryable<T> all over your app because you are trying to build applications fast and don't spend the time to abstract out your data access layer), then, yes, I agree with the EF team: there is no general solution that is both general and good. I could hack a solution by transforming expression trees prior to EF evaluating the query, but that also would require every caller to be deliberate and tag the final expression. I could also write a Roslyn analyzer that warns if queries with ToList do not include a TagWith operation, but that also seems hacky.

  IQueryable<Person> All() {
    return DbSet<Person>().TagWith($"{GetTypeAndMethodName()}");
  }

[ajcvickers]

@m60freeman I don't think so, no.

[jzabroski]

@ajcvickers Why not?

[ajcvickers]

Because the feature request here is to tag the location in code where the query is created, wherever that might be. I don't see anything in the discussion that addresses this.

[jzabroski]

@ajcvickers Why doesn't the following address this? It's not perfect since it requires manual plumbing, but I believe that manual plumbing can also be automated with a leap of faith.

Is your distaste that it doesn't log the final, final endpoint? For example, if I have an Mvc WebApi Controller that is api/Persons/All route, you want to log that as well?

class PersonRepository : IRepository<Person> {
  DbContext _dbContext;
  PersonRepository(DbContext dbContext) {
    _dbContext = dbContext;
  }
  private static string GetTypeAndMethodName([CallerMemberName] string callerName = null)
  {
    return $"{typeof(PersonRepository).Name}.{callerName}";
  }
  IQueryableResult<Person> All() { // assume IQueryableResult is an abstraction which blocks adding method chains to an IQueryable<T>
    return DbSet<Person>().TagWith($"{GetTypeAndMethodName()}"); // this will tag the query with "PersonRepository.All"
  }
}

[ajcvickers]

@jzabroski Of course you can do that, but that code assumes you are using a repository and adding code to it to add tags. This issue is about automatically figuring out where the query comes from without any code changes on the application's part.

[jzabroski]

@ajcvickers Literally, you are correct. In practice, let's ask @m60freeman if that is what he thinks he really needs.

I wonder if @m60freeman would be happy as a DBA going to his C# engineers and pointing at my code sample and saying, "Guys and gals, if you did this for me, I could really help you pinpoint some major performance issues in your code a lot faster." I believe that is the user story that really needs capturing, and my point is you've already done it you just aren't properly educating end users about this feature because you are trying to build a perfect solution to everything.

You're an amazing engineer - I've read through your code. You have me beat six ways from Sunday. But do you really think this problem requires a perfect solution to work for most companies?

[ajcvickers]

@jzabroski Nope. That's why this issue is closed.

[m60freeman]

@ajcvickers <https://github.com/ajcvickers>, @jzabroski <https://github.com/jzabroski>: Having a solution that is completely automatic would be ideal. A solution that works if you insert some specific code and doesn't impact behavior in a negative way if such code is not inserted would certainly be acceptable to me.

…

On Tue, Apr 2, 2019 at 2:49 PM John Zabroski ***@***.***> wrote: @ajcvickers <https://github.com/ajcvickers> Literally, *you are correct*. In practice, let's ask @m60freeman <https://github.com/m60freeman> if that is what he thinks he really needs. I wonder if @m60freeman <https://github.com/m60freeman> would be happy as a DBA going to his C# engineers and pointing at my code sample and saying, "Guys and gals, if you did this for me, I could really help you pinpoint some major performance issues in your code a lot faster." I believe that is the user story that really needs capturing, and my point is *you've already done it* you just aren't properly educating end users about this feature *because you are trying to build a perfect solution to everything* . You're an amazing engineer - I've read through your code. You have me beat six ways from Sunday. But do you really think this problem *requires* a perfect solution to work for most companies? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGHjPjLb88tjVlb_hzcx7ZsYTH7KgoGtks5vc6Y-gaJpZM4ZMR_F> .

[jzabroski]

@ajcvickers : @m60freeman Did not say "automatically do this for me". He asked for an option to have every LINQ statement default to using the current class and method. I actually don't think that's too hard - I've not inductively defined my boilerplate code and scraped the boilerplate through dependency injection and some abstraction, but the seed of an idea is absolutely there.

Look, I do similar things with ILog<T> abstractions and dependency injection. I'm lazy and don't like writing private Ilog _log = LogManager.GetLogger<MyClass>(); so instead I inject it. My DI framework just needs to know the request scope at time it resolves the dependency.

[ajcvickers]

@jzabroski So what are you suggesting we do in EF?

[jzabroski]

Literally just document how to take advantage of the infrastructure. It's product management work - not coding work.

[ajcvickers]

@jzabroski For that, please file a documentation issue on this page: https://docs.microsoft.com/en-us/ef/core/querying/tags

[simeyla]

@m60freeman Unfortunately I don't see the point of any of this unless the comment added by TagWith(...) would actually end up IN query store query_sql_text column. Currently the comment is added to the beginning of the statement - which means it just gets stripped off.

How query store collects data

Comments and spaces before and after the query text are ignored. Comments and spaces inside text aren't ignored. Every statement in the batch generates a separate query text entry.

Did you ever come up with anything? I've been after this functionality for 12 years!

See also: https://github.com/MicrosoftDocs/sql-docs/issues/2789

[jzabroski]

@simeyla I was imagining using SolarWinds Database Performance Analyzer or SQL Sentry in order to catalog running requests and their associated SQL. I was not imagining using Query Store. While I have Query Store enabled, I have found it of limited usefulness in my environments. If you've been after this functionality for 12 years and haven't considered either of the two above tools, then please try now. I found SolarWinds on-boarding SQL experts to also be willing to help get started. It is the product I use and I mostly love it (although, recent product enhancements aren't implemented ideally and seem to be hacky).

[m60freeman]

@ajcvickers This may have greater visibility once SQL Server 2022's parameter sensitive plan optimization renders most monitoring software and scripts unable to associate queries with the stored procedures that contain them. See this blog post for details. Here is another one. Fixing the EF problem won't fix the PSPO problem, but I thought it would add some perspective as to why the lack of traceability is a big deal.

[velniaszs]

@jzabroski Query Store has its benefits - it is free for developers to use locally or in Azure and it is light weight, not affecting performance of the SQL Server. We do not need to rely on third party tools. Also Azure Index recommendations come directly from Query Store data, so when we start investigation why SQL engine thinks an index is needed we rely on Query Store.
@ajcvickers About EF TagWith functionality. It looks very close to what would be useful if it would be captured in Query Store.
Not sure if technically this would be complicated to implement, but maybe it would be possible to transfer a placement of the comment after the first word? i.e.

From :

--This is my spacial query! 

SELECT Id, Name, LastName, Email FROM People 

TO:

SELECT /*This is my spacial query! */Id, Name, LastName, Email FROM People