Efcore: 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 https://github.com/aspnet/EntityFrameworkCore/issues/12669#issue-341120196

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 [e-mail]
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
https://github.com/aspnet/EntityFrameworkCore/issues/14134#issuecomment-479144205,
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).