What's New: Interception #4052
What's New: Interception #4052
Conversation
ajcvickers
force-pushed
the
F1180921
branch
2 times, most recently
from
a213c15
to
20394e6
Compare
ajcvickers
force-pushed
the
F1180921
branch
from
20394e6
to
db468cd
Compare
There was a problem hiding this comment.
@ajcvickers should we be adding such long/comprehensive sections to What's New at this point, as opposed to just adding the real docs for them? We have to do the real docs anyway now, it seems like a lot of duplication...
Apart from that mainly see the comments on async interception for ConnectionOpening and retrieving the connection string/token asynchronously, we should be careful not to show something that would lead people to be very inefficient...
| b => b.UseSqlServer(); | ||
| ``` | ||
|
|
||
| One of the `IDbConnectionInterceptor` methods can then be implemented to configure the connection before it is used. `ConnectionOpeningAsync` is a good choice, since it can perform an async operation to obtain the connection string, find an access token, and so on. For example, imagine a service scoped to the current request that understands the current tenant: |
There was a problem hiding this comment.
Doing an async operation every time a connection is opening to obtain a connection string sounds pretty bad... Fetching an access token is also likely to be an anti-pattern in most cases, unless the user makes sure that caching is implemented underneath (i.e. that actual I/O only happens once every hour etc.).
(I think we've discussed this in the past - my personal opinion is that it's better to have some service managing the periodic refreshing of the token via a token, and for the interceptor to only synchronously fetch the latest token from the service.. Documenting (and even providing) the async option feels like a pit of failure to me)
In any case, if we're going to discuss these usages, it's probably a good idea to at least mention that doing an async operation on every opening event is a really bad idea.
There was a problem hiding this comment.
Doing an async operation every time a connection is opening
It only happens the first time the connection is opened. So, no more times that currently happens when it is done in UseSqlServer.
doing an async operation on every opening event is a really bad idea.
Will do this, but just because it's an async call doesn't mean it has to be an async operation. It might, for example, only actually do something async once per hour, or whatever, when it needs to refreshed something cached. Or it might only be async if a cache of connections strings doesn't have an entry for some given client. Regardless, it is common that people need to, at least sometimes, do something async when getting a connection string, and I think it makes sense to show this. Otherwise people will keep doing things like sync-over-async in the constructor, which is worse.
There was a problem hiding this comment.
OK. I specifically have never seen a need for remotely accessing connection strings (as opposed to loading them on application start), unless maybe there's a facility that integrates an auth token into them (it always goes back to the auth token).
Yeah, I agree that if your async auth token code already includes a cache, it's OK to call this from the interceptor's async method. However, IIRC the last time I looked into this, it was very unclear/undocumented if e.g. the Azure API did, and I suspect lots of users end up doing actual async on every opening.
The optimistic side of me hopes that not providing the async hook would nudge people towards the right solution; the pessimistic/realistic agrees that it may just nudge them towards sync-over-async.
Anyway, maybe worth just adding a note ("verify that you have caching and are not doing I/O every time, otherwise perf will suffer greatly").
There was a problem hiding this comment.
How's this?
Warning
Performing a asynchronous lookup for a connection string, access token, or similar every time is is needed can be very slow. Consider caching these things and only refreshing the cached string or token periodically. For example, access tokens can often be used for a significant period of time before needing to be refreshed.
There was a problem hiding this comment.
Yep, looks good (just fix the is is)
| { | ||
| if (string.IsNullOrEmpty(connection.ConnectionString)) | ||
| { | ||
| connection.ConnectionString = (await _connectionStringFactory.GetConnectionStringAsync(cancellationToken)); |
There was a problem hiding this comment.
It's really hard for me to imagine a situation where someone should do this...
There was a problem hiding this comment.
See previous comment.
| ```output | ||
| info: Microsoft.EntityFrameworkCore.Database.Command[20101] | ||
| Executed DbCommand (26ms) [Parameters=[@p0='?' (Size = 4000), @p1='?' (Size = 4000), @p2='?' (Size = 4000), @p3='?' (Size = 4000)], CommandType='Text', CommandTimeout='30'] | ||
| SET STATISTICS IO ON;SET IMPLICIT_TRANSACTIONS OFF; |
| > [!TIP] | ||
| > The code shown here comes from [QueryStatisticsLoggerSample.cs](https://github.com/dotnet/EntityFramework.Docs/tree/main/samples/core/Miscellaneous/NewInEFCore7/QueryStatisticsLoggerSample.cs). | ||
|
|
||
| Finally, let's create two interceptors that work together to send SQL Server query statistics to the application log. To generate the statistics, we need an <xref:Microsoft.EntityFrameworkCore.Diagnostics.IDbCommandInterceptor> to do two things. |
Co-authored-by: Shay Rojansky <roji@roji.org>
Co-authored-by: Shay Rojansky <roji@roji.org>
Co-authored-by: Shay Rojansky <roji@roji.org>
Co-authored-by: Shay Rojansky <roji@roji.org>
Co-authored-by: Shay Rojansky <roji@roji.org>
I think so. Different audiences. This is for, "I know EF Core 6.0; show me everything new in EF7." Main docs are for, "I want to use an interceptor for something, show me how to do it. I don't care if it's new or not." All this content is useful in both places. |
|
Well, you know my opinion... Assuming we have comprehensive docs (which we need to in any case), the depth of the what's new sections seems way beyond "just show me the new 7.0 stuff". I personally think the huge page size is pretty scary and may be deterring users from actually reading it. And writing long comprehensive docs twice for the same feature, well... We all have our hobbies :) But anyway, we've discussed this in the past - am OK with merging etc. |
No description provided.