Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
m60freeman opened this issue Dec 10, 2018 · 22 comments

Comments

@m60freeman
Copy link

commented Dec 10, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 15, 2018

@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

This comment has been minimized.

Copy link
Author

commented Dec 17, 2018

@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

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

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

@smitpatel

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

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

@m60freeman

This comment has been minimized.

Copy link
Author

commented Dec 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

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

This comment has been minimized.

Copy link

commented Apr 1, 2019

@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

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

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

@jzabroski

This comment has been minimized.

Copy link

commented Apr 2, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@m60freeman I don't think so, no.

@jzabroski

This comment has been minimized.

Copy link

commented Apr 2, 2019

@ajcvickers Why not?

@ajcvickers

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

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

This comment has been minimized.

Copy link

commented Apr 2, 2019

@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

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@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

This comment has been minimized.

Copy link

commented Apr 2, 2019

@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

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

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

@m60freeman

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

@jzabroski

This comment has been minimized.

Copy link

commented Apr 2, 2019

@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

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

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

@jzabroski

This comment has been minimized.

Copy link

commented Apr 2, 2019

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

@ajcvickers

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.