-
Notifications
You must be signed in to change notification settings - Fork 607
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
MiniProfiler v4 #144
Comments
If time permits, you may want to add "MiniProfiler EF6 and batch queries #130" to your list of v4 changes. I'll try to add some of the information you requested on that bug's thread, but I believe it is going to require some low level change in the connection facility to handle this. |
I got I'll try and get the sample app updated to MVC5 (not to be confused with ASP.NET 5/MVC Core) tomorrow and make sure that's all running. VS 2017 doesn't support loading/recognizing the MVC4 type GUIDs, so they're not going to work here. But I don't plan on supporting MVC4 in MiniProfiler v4 (v3 can continue to do so), only MVC5+. So updating the samples in this branch is logical. |
Async persistence is in: 0b0c8b1 |
Do you think any of the most important tasks needed for a stable release can be done by the community or do you think all of them requires some kind of design first? I'm asking because I would like to help but I'm not sure how I can do that without stepping on your toes. |
@akamud MongoDB really needs some love (I don't use it at all), the rest is a bit fluid at the moment as I shift major pieces around. Unit tests are another, but they may need some fundamental work for netstandard. Longer version: If anyone wants to contribute a lot of async tests, they'd of course be welcome - it's possible all of that is broken. I'm getting existing stuff working in I'm also adding a Redis provider now...since we'll want that. It'll contain both storage and provider bits for SE.Redis, if I can figure out the provider plug cleanly. Storage is more straightforward and makes a lot of sense as async etc. |
Update: moved RavenDB/MongoDB to post-v4.0, as they are separate libraries and don't need to have a simultaneous release anyway. MongoDB needs a lot of love (that I won't have time for) and RavenDB needs some love, possibly with v4 changes to allow proper profiling with tree attachment at the call site. I've also added a StackExchange.Redis package to the v4 plan. We're essentially already doing this in Stack Overflow with MiniProrifler v3, so the hardest part of this is: what should that package be named?
I'm leaning towards |
For anyone curious or anxious to test - I'm prepping package releases for an alpha, |
A few clicks and I got this working. A pleasant surprise, and greatly missed in .NET Core land. Thank you! |
Is it possible to use v4 with an ASP.NET Core website on top of net46? Ain't working with my current setup, but I probably got something wrong... |
@jasselin I'm on .NET Core but I just copied the relevant stuff from Startup.cs and the Views from here and it came right up: https://github.com/MiniProfiler/dotnet/tree/master/samples/Samples.AspNetCore Make sure you have all the stuff in the right spot in Startup::Configure and Startup::ConfigureServices... don't just tack it to the top or bottom. |
Configuration should not be a problem. Actually, I'm having trouble just installing the right nuget packages... |
@jasselin This was my fault, I forgot to add back the Can you please try that and let me know what if any restore issues you're hitting? The testing is much appreciated. |
@NickCraver Package restore works fine now. I'll have to make some changes on my side to make everything work again the way it used to for my specific needs. That's quite a rewrite you did there :) |
@jasselin Awesome - I'll take every bit of feedback you've got time to give. Thanks! |
@NickCraver Dependencies does not seem to get pulled automatically. I had to reference MiniProfiler.AspNetCore and MiniProfiler.Shared manually to be able to call UseMiniProfiler(). It is the wanted behavior? This error also shows up quite often: |
@jasselin hmm looks like package builds are trying to depend on 4.0.0+ (which doesn't exist yet), rather than the alpha release. I'm digging into how to fix it now. |
@jasselin It looks like this is a bug in NuGet (here: NuGet/Home#4337) - I'll see if we can get it resolved or if there's a workaround. At the moment, yes: reference both of them, unfortunately. That's definitely not the intended behavior but we're dealing with a tooling bug :( |
@jasselin Alrighty, now there's a very nasty workaround in place for that issue. If you try and get Also: I've got to change the versioning here so that the alpha number isn't astronomical with CI builds. |
I've been testing v4 in an MVC5/EF6 context. It appears that EF6 requires DbCommand instances to implement ICloneable (here and here). I was able to work around this by adding a compiler directive in ProfiledDbCommand to implement ICloneable if the target is .NET 4.6. I looked through the rest of the reference source for EF6 and wasn't able to find uses of ICloneable for DbConnection. If you'd like, I can put a pull request together for this change. Thanks!
|
@brdmllr Thanks for the heads up - I'm doing some major changes (simplifications) to how EF6 works and adding EF Core support as well...may go in tonight. I just need to change up a few more things and test a bit then should have a new release. We're also changing the versioning for all of our open source libs to be consistent and easier. Stay tuned for new builds soon. |
Thanks for this, Nick. I've seen the work you're doing driving the various Microsoft database teams. They're a little off in the weeds so having someone raise the issues the way you are is really helpful. Thanks again. |
@brdmllr I've pushed @chadwackerman <3 - glad I can help. This update includes fixes for EF6 and I've also pushed an alpha of MiniProfiler.EntityFrameworkCore for Entity Framework Core support via DiagnosticListener. Undoubtedly it'll need some changes and I'm pushing Microsoft for many changes and enhancements to Diagnostics overall. I'll try and get those PRs in for Entity Framework (dotnet/efcore#8007) this week, but there's no rush since they won't appear until the Next up, I'll change the versioning to be something sane. The new format will be |
For those that care, the next build will include .NET 4.5 support for v4: 4297dcf via some polyfills for New version numbers as mentioned in the last comment are up on NuGet (but not .NET 4.5, yet - likely this week...it's on MyGet). Changes coming:
What other pieces are people missing/wanting? I'm admittedly short on time, but trying to get this moved along as I can. I really, really, really appreciate all the community help lately. You guys and gals make me proud. I can't thank you enough. |
This is an awesome project @NickCraver and the updates look great! I saw on the roadmap releasing the redis storage provider. That is something that has intrigued us greatly and I had planned on writing my own till I saw you had one already planned. That's the only thing I'd love to see - but not a dealbreaker one bit. Thanks again for all the work you do! |
@lemkepf Lucky for you, @jdaigle has already been working on that. I think we need to add a few more options (like a connection string connection and it handling the multiplexer, etc.) But it'll be in v4 for sure. Code's here: https://github.com/MiniProfiler/dotnet/blob/master/src/MiniProfiler.Providers.Redis/RedisStorage.cs Alpha is here: https://www.nuget.org/packages/MiniProfiler.Providers.Redis/4.0.0-alpha6-52 It takes an IDatabase currently, but thinking we should add a few configuration overloads. Do you have any thoughts on that front? |
SWEET. Let me pull it down this afternoon and I'll get back to you! |
@NickCraver Two thoughts:
Also, I tried testing the alpha implementation and couldn't get anything to store into Redis. I'm going to pull down the source and try it again. I'll get back to you.... |
@lemkepf Talked about this with Marc earlier - I'll try and do it tonight. I plan on adding 2 overloads: one with a connection string, and one with a ConfigurationOptions (from StackExchange.Redis), to allow easy configuration. I'll also put it though some testing tonight as well, though setting up the skips may take a bit longer (they're a bit involved with xunit to do "properly" when redis isn't running). Are you able to easily test the MyGet feed? I'll get a build with overloads popped up there tonight for sure. |
@NickCraver Yea, I can test from the MyGet feed today if it's up. |
@lemkepf sorry, fell asleep at a keyboard before finishing last night. RedisCache with new constructors and initial tests is now on MyGet: https://www.myget.org/gallery/miniprofiler |
I couldn't figure out how to secure this in ASP.NET Core. I set up a middleware handler to check the user and call MiniProfiler.Stop(discardResults: true) but it didn't seem to have an effect. Is there an efficient way to set this up? |
@chadwackerman The authorization functions are on the options (per ASP.NET Core conventions) now. They still take a app.UseMiniProfiler(new MiniProfilerOptions
{
RouteBasePath = "~/profiler",
ResultsAuthorize = request => GetUser(request).CanSeeMiniProfiler,
ResultsListAuthorize = request => GetUser(request).CanSeeMiniProfiler,
}); |
This really was missing, related to #144
@chadwackerman I've added some sorely lacking documentation to the samples project, I'd love to know if anything is unclear or you think we should expand greatly on what's there. |
@NickCraver The notes you made in the sample code are more than sufficient. (Not sure how I missed those methods.) A walkthrough adding it to a project, with and without Identity, would be useful for people just getting started. But I'm sure a blogger will take care of that. |
Disclaimer: I have a strong feeling I'm being thick here. I can't get it working for my .net core 1.1 mvc web app. I've posted in SO since that seems a better place than here and since it's almost def. my fault - https://stackoverflow.com/questions/44434904/miniprofiler-not-showing-up-in-asp-net-core Perhaps the asp.net core docs could use a "quickstart" walkthrough? |
All, If anyone has time, can they please take a look and make suggestions? |
A new alpha8 is up on NuGet for testing. ASP.NET Core especially received a big configuration overhaul to match what a user should expect there and line up with the isms in that framework. Please let me know if you have any issues. The docs have been updated to reflect the new config: http://miniprofiler.com/dotnet/AspDotNetCore |
I see that "Elasticsearch" is listed in the new features list. Was any decision ever made on supporting it? Considering that it is not SQL, I'm curious how it would be represented. I'm involved with a project that is moving to Elasticsearch (switching from Solr) and it would be interesting to have insight into what is happening with the behind the scenes search calls. Count me as a vote of interest, although not a have to have this to live type. |
@StuffOfInterest - The elasticsearch note listed here is for a storage provider, not for visual display. The storage providers are where miniprofiler stores it's logs. If you want to visually see what's going on with elasticsearch it's pretty easy to wrap your calls with miniprofiler magic to show them. Here is some sample code to add a logger when you setup your elasticsearch settings:
|
@NickCraver Please consider adding profiling for ElasticSearch as well. It is used by a lot of people. It will be a huge benefit for our community to have ElasticSearch integration. |
@rahamohebbi Which elastic search library are you using to access Elastic? The difference between Elastic and SQL Server for example is that Elastic has many possible clients whereas SQL Server we're using the official model to plugin (ADO.NET). The profiling bits would have to be specific per provider. As an example, we use a custom library here at Stack to deal with Elastic with minimal overhead. I'm not sure about supporting many clients, as that gets to be high cost fairly quickly, but if we can figure out a pluggable way to deal with this or a more plug-in friendly approach (e.g. some extension methods to help) with clients that allow hooks like @lemkepf notes above, then we could make that a copy/paste operation for many with minimal code, and all of it in docs. I'd say that's a best first step in any case when supporting many of any platform. As an example, what if we had an extension that easily allowed this: elasticSettings = elasticSettings
.DisableDirectStreaming()
.OnRequestCompleted(details =>
{
decimal? totalDuration = (decimal?)details?.AuditTrail?.Sum(c => (c.Ended - c.Started).TotalMilliseconds);
MiniProfiler.Current.AddCustomTiming("elastic", totalDuration, details.Uri, details.HttpMethod);
}); |
@NickCraver thanks for info. I like your idea. It would be great if we can come up with a simple code that could be easily copy pasted into a project. At my company we use the official client library from ElasticSearch called Nest. I personally haven't seen anyone to use a different one. They have a nice dotnet core support as they also update it frequently to match the changes they make to the server. I have a question about the code snippet that you have pasted above. If we add the custom timing like that, would it be displayed on the mini profiler's UI widget ? |
@rahamohebbi Yep, it'd be exposed :) |
MiniProfiler v4 is now on NuGet and I've added release notes here: https://miniprofiler.com/dotnet/Releases Thanks for all for helping make v4 possible. I'm going to close this out so items anyone finds are in distinct issues and not lost in a mega thread here :) |
I'm working on v4 of MiniProfiler as time allows, but an overall progress checklist would probably be helpful. If anyone has comments, suggestions, reminders of breaking changes we should make now, etc. - please, chime in.
You can track progress
on the v4 branch herenow [in the masterbranch]: (https://github.com/MiniProfiler/dotnet). V3 has been branched off for any servicing here.The library layout I currently have splits the primary libraries into
MiniProfiler
(current lib),MiniProfiler.AspNetCore
, andMiniProfiler.Shared
(referenced by both).The storage for things like SQL Server are in
MiniProfiler.Providers.SqlServer
, and evenMiniProfiler.Providers.SqlServerCe
. Now the SQL Server profilers are free (all ADO.NET providers are, being based onDbConnection
), so it's essentially free to include "in the box" inMiniProfiler.Shared
with no additional references. So I maintain it there and make life easier. The SQL Server libs are only needed for storage.In other cases like
MiniProfiler.Providers.RavenDB
,MiniProfiler.Providers.MongoDB
, etc. they'll have both a profiler and a storage provider on the package. I think this makes the most sense of the available options and tradeoffs in reference weight. That means providers and platforms that support variousnetstandard
levels can just work on the most things possible.New major features:
Trailer
headers<mini-profiler>
tag helper<profile name="My Step">
tag helperasync
)Project System:
.nupkg
s building with new .csprojTarget Frameworks:
netstandard1.5
(for all non-System.Web
)net46
(for all)API Changes (breaking):
Name
to SQL Storage providers (Added Name to SqlServerStorage #104)IDisposable
toTiming
returns (Added a Step extension to allow a step to specify its parent #38)ASP.NET Core Support (See #116):
netstandard
build)ProfilingActionFilter
ProfilingViewEngine
Setup.cs
Profiler Providers:
net46
)netstandard
)Cleanup:
Storage Providers:
net46
)netstandard
)net46
)StackExchange.Redis
)async
love for all ([Feature] Use v4.5 and async sql profiling #129)Async/Await (See #127):
IStorage
additions, likelyIAsyncStorage
) (0b0c8b1)Other Issues:
IsTrivial
impl inTiming
Post-v4.0 (main libraries):
net46
)netstandard
)net46
, Mongo 2.x driver)netstandard
, Mongo 2.x driver)I'll try to keep this updates as things progress. I'm hoping to get an alpha out in the next few weeks, but there is a lot to do yet. The packages from new
.csproj
may be a hard tooling blocker. I hope not, working with the Microsoft teams on that one.Breaking Changes:
.tmpl
are still replaceable, theshare
andincludes
templates are now much more optimized code. Given the very few people customizing these (was anyone using those pieces?), they certainly weren't worth the performance tradeoffs. If there's a loud demand for them to come back, we'll find a more efficient way to do it.Name
field has been added to the SQL Server Profiler storagenetstandard1.5
) required (due to lack of the framework bits needed to really make async profiling work correctly, .NET 4.5.x uses a polyfill forAsyncLocal<T>
and below that will not be supported, v3 will remain available of course).MiniProfiler.Step()
andMiniProfiler.StepIf()
methods now returnTiming
(the same previous underlying type) instead ofIDisposable
(e723bdc).IProfilerProvider
replaced withIAsyncProfilerProvider
(which addsStopAsync(bool discardResults)
) (0b0c8b1)IStorage
replaced withIAsyncStorage
(which addsListAsync
,SaveAsync
,LoadAsync
,SetUnviewedAsync
,SetViewedAsync
, andGetUnviewedIdsAsync
) (0b0c8b1)ProfiledDbCommand
,ProfiledDbConnection
, andSimpleProfiledCommand
no longer implementICloneable
(723d9d2)MiniProfiler.Settings.(AssembliesToExclude|TypesToExclude|MethodsToExclude)
changed fromIEnumerable<string>
toHashSet<string>
(for performance - b826225)MiniProfiler.ToJson(MiniProfiler profiler)
is nowprofiler.ToJson()
(instance method - d4c0d4f)IProfilerProvider.Start(ProfileLevel level, string sessionName = null)
MiniProfiler.Settings.ExcludeStackTraceSnippetFromSqlTimings
MiniProfiler.Settings.UseExistingjQuery
MiniProfilerExtensions.Inline<T>(this MiniProfiler profiler, Func<T> selector, string name, ProfileLevel level)
MiniProfilerExtensions.Step(this MiniProfiler profiler, string name, ProfileLevel level)
The text was updated successfully, but these errors were encountered: