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

New approach to profile EF6, using an Interceptor #44

Closed
wants to merge 1 commit into from
Closed

New approach to profile EF6, using an Interceptor #44

wants to merge 1 commit into from

Conversation

felipepessoto
Copy link
Contributor

New approach to profile EF6, using an Interceptor
New Feature - Command String includes parameters values and details

New Feature - Command String includes parameters values and details
@yellis
Copy link
Member

yellis commented Apr 2, 2014

@fujiy thanks for the PR.

Can you please make a case why this approach is better than the existing approach used in this project? What does it add (feature/performance/maintainability/usability) that we don't have already?

Also, while the current approach wraps all of the EF commands as ProfiledCommands, if I am reading your code correctly, it looks like you are registering all EF6 db interactions as custom timings, and are writing your own timing logic for this. This seems like a less than ideal scenario (since EF6 db interactions do not feel to me like they belong in custom timings).

@felipepessoto
Copy link
Contributor Author

Hi @yellis

The main reason I did it is because its conflicting with other libraries that also wraps DbProviderServices, like EFProf: http://community.miniprofiler.com/t/miniprofiler-efprof-doesnt-work/239

MiniProfiler was assuming that DbProviderServices should have a Instance property.

I see this approach as a Hack, what is explainable, since was the only way to do it. (EFProf also do it)

But now, EF Interceptors came to solve this problem.

Answering your question:

1-) I think this new approach is just more maintainability and correct, but I didn't any enhancement but Parameters Info (http://blog.fujiy.net/Upload/Post10210/example.png)
I wrote the EF6 code based on RavenDb

2-) I never read MiniProfiler code until now, So maybe I didn't take the best approach, but using Interceptor doesn't seem make sense to use ProfiledCommand.

@yellis
Copy link
Member

yellis commented Apr 22, 2014

I hear what you are saying about using an "explainable hack", and the benefits of getting away from it.

However, I don't think that we would consider doing that unless we were still treating Sql Commands (since that is what EF is producing) as SqlTimings, and not as CustomTimings (which they most definitely are not). Perhaps if the code was changed to use your new Initialize method, but storing the commands as SqlTimings. This is something that ProfiledDbCommand does very well right now (used by the current EF implementation), but which would require a good deal of new code (much of it near duplicate) in order to implement without using ProfiledDbCommand.

So I don't know if it is really worth it, especially considering that the current method does work for almost every case, and this initialization method is only causing issue when MiniProfiler is being used with a different EF profiler at the same time (something that we would be happy to avoid if easily possible, but is also not a scenario that will come up so often).

@dixon your thoughts?

@felipepessoto
Copy link
Contributor Author

Maybe I can use SqlTimings, but isn´t it a wrapper to CustomTiming, that helps the old approach and will just add complexity to new approach?

The new approach is much more simpler to mantain and smaller code

@yellis
Copy link
Member

yellis commented May 11, 2014

@fujiy I appreciate your approach. However, as the current method we are using for EF works for all scenarios other than a conflict with a different profiler, and also supports add ons like spatial support, I don't see us moving forward rewriting the logic for this at the current time.

@yellis yellis closed this May 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants