-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Make ILoggerFactory a scoped service #14706
Conversation
Issue #14698 Main changes: * ILoggerFactory is still captured in AddDbContext, but as a wrapped scoped factory * Same thing can be done with UseLoggerFactory * No longer pathological to pass a different instance each time * IMemoryCache is no longer captured from AddDbContext * Can still be passed to UseMemoryCache * IRawSqlCommandBuilder and IMigrationsSqlGenerator have been made scoped * Validation and query compilation, method call translators, and a few more now accept the logger to use
16f33f1
to
909f473
Compare
@@ -59,7 +59,7 @@ public class ModelExpressionApplyingExpressionVisitor : RelinqExpressionVisitor | |||
((QueryModelGenerator)queryModelGenerator).EvaluatableExpressionFilter, | |||
_parameters, | |||
_queryCompilationContext.ContextType, | |||
_queryCompilationContext.Logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably QCC can directly expose Logger of DbLoggerCategory.Query since it always ask for same logger. I am fine either way though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it also needs the Database logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe that's somewhere else. :-)
Expression Translate([NotNull] MethodCallExpression methodCallExpression); | ||
Expression Translate( | ||
[NotNull] MethodCallExpression methodCallExpression, | ||
[NotNull] IDiagnosticsLogger<DbLoggerCategory.Query> logger); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly not fan of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could create new MethodCallTranslators each time they are needed and inject into the constructor that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be a way. I prefer to put it in constructor rather than method itself. If translator needs it, they can ask for it. They don't have to understand the method args when they don't even need it at times. We can re-visit when I integrate logging into new pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, let's revisit then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making the migrations sql generator scoped might help some multitenant scenarios... Vaguely sounds like something that’s been asked for in the past |
@bricelam Yeah, I remembered that too, which is one of the reasons I went the scoped route for it. However, I also remember that the people who wanted this looked like they were taking an approach that wasn't very robust anyway. So not sure if this is good or not in that respect. |
Issue #14698
Main changes: