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

MiniProfiler v4 #144

Closed
41 of 43 tasks
NickCraver opened this issue Nov 30, 2016 · 39 comments
Closed
41 of 43 tasks

MiniProfiler v4 #144

NickCraver opened this issue Nov 30, 2016 · 39 comments
Assignees
Milestone

Comments

@NickCraver
Copy link
Member

NickCraver commented Nov 30, 2016

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 here now [in the masterbranch]: (https:/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, and MiniProfiler.Shared (referenced by both).

The storage for things like SQL Server are in MiniProfiler.Providers.SqlServer, and even MiniProfiler.Providers.SqlServerCe. Now the SQL Server profilers are free (all ADO.NET providers are, being based on DbConnection), so it's essentially free to include "in the box" in MiniProfiler.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 various netstandard levels can just work on the most things possible.

New major features:

Project System:

  • Convert to new .csproj (VS 2017)
  • Port tests to XUnit
  • Get all tests passing
  • Get .nupkgs building with new .csproj

Target Frameworks:

  • netstandard1.5 (for all non-System.Web)
  • net46 (for all)

API Changes (breaking):

ASP.NET Core Support (See #116):

  • MiniProfiler.Shared (netstandard build)
  • MiniProfiler.AspNetCore library
  • Middleware
  • Results Routes
  • Sharing Routes
  • JS embeds
  • ProfilingActionFilter
  • ProfilingViewEngine
  • Helpers for easy Mvc profiling in Setup.cs
  • Profiler ID Headers

Profiler Providers:

  • System.Data (net46)
  • System.Data (netstandard)

Cleanup:

  • Unify /ui/ directory storage across libraries
  • StrongName all libraries possible (some third-party are not signed)

Storage Providers:

Async/Await (See #127):

Other Issues:

Post-v4.0 (main libraries):

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:

  • UI templating has been removed. While Stylesheets and the .tmpl are still replaceable, the share and includes 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.
  • A Name field has been added to the SQL Server Profiler storage
  • .NET 4.5+ (or netstandard1.5) required (due to lack of the framework bits needed to really make async profiling work correctly, .NET 4.5.x uses a polyfill for AsyncLocal<T> and below that will not be supported, v3 will remain available of course).
  • MiniProfiler.Step() and MiniProfiler.StepIf() methods now return Timing (the same previous underlying type) instead of IDisposable (e723bdc).
  • IProfilerProvider replaced with IAsyncProfilerProvider (which adds StopAsync(bool discardResults)) (0b0c8b1)
  • IStorage replaced with IAsyncStorage (which adds ListAsync, SaveAsync, LoadAsync, SetUnviewedAsync, SetViewedAsync, and GetUnviewedIdsAsync) (0b0c8b1)
  • ProfiledDbCommand, ProfiledDbConnection, and SimpleProfiledCommand no longer implement ICloneable (723d9d2)
  • MiniProfiler.Settings.(AssembliesToExclude|TypesToExclude|MethodsToExclude) changed from IEnumerable<string> to HashSet<string> (for performance - b826225)
  • MiniProfiler.ToJson(MiniProfiler profiler) is now profiler.ToJson() (instance method - d4c0d4f)
  • Many [Obsolete] methods removed:
    • 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)
@StuffOfInterest
Copy link
Contributor

StuffOfInterest commented Nov 30, 2016

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.

@NickCraver
Copy link
Member Author

NickCraver commented Dec 1, 2016

I got .nupkg figured out tonight. Latest comments added a build script building the libs that are ready (all except EntityFramework and MongoDB). You can find the mapping info here on nuget.org.

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.

@NickCraver
Copy link
Member Author

Async persistence is in: 0b0c8b1

@akamud
Copy link

akamud commented Jan 23, 2017

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.

@NickCraver
Copy link
Member Author

@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:
I'm getting close on the other pieces are time allows, but some bits will be very tricky, like async/threading support. In theory, async works right now, but I need far more tests for this. I also need to either add netstandard support to the test suite (which is a lot of #ifdef).

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 netstandard (ASP.NET Core) first, then working more on async/threading. As I go though, I'm re-working both handlers because we want as much as possible shared between them. It's a hard line to figure out though, because of the web types split (e.g. 2 different HttpContexts). So DRY coding isn't so awesome.

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.

@NickCraver
Copy link
Member Author

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?

  • MiniProfiler.Providers.SERedis?
  • MiniProfiler.Providers.StackExchangeRedis?
  • MiniProfiler.Providers.StackExchange.Redis?
    Other?

I'm leaning towards MiniProfiler.Providers.StackExchange.Redis since MiniProfiler.Providers.<packageName> is a reusable convention we could be fairly consistent about.

@NickCraver
Copy link
Member Author

For anyone curious or anxious to test - I'm prepping package releases for an alpha, v4 is now merged into master: https:/MiniProfiler/dotnet/

@chadwackerman
Copy link

A few clicks and I got this working. A pleasant surprise, and greatly missed in .NET Core land. Thank you!

@jasselin
Copy link

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...

@chadwackerman
Copy link

@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:/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.

@jasselin
Copy link

Configuration should not be a problem. Actually, I'm having trouble just installing the right nuget packages...

@NickCraver
Copy link
Member Author

@jasselin This was my fault, I forgot to add back the net46 builds after finishing here. You should need to reference only the MiniProfiler.AspNetCore.Mvc package (4.0.0-alpha23 and above contains a net46 build). It'll pull in the others needed as dependencies.

Can you please try that and let me know what if any restore issues you're hitting? The testing is much appreciated.

@jasselin
Copy link

jasselin commented Mar 20, 2017

@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 :)
Thanks for the quick reply.

@NickCraver
Copy link
Member Author

@jasselin Awesome - I'll take every bit of feedback you've got time to give. Thanks!

@jasselin
Copy link

@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:
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\MSBuild\Sdks\Microsoft.NET.Sdk\build\Microsoft.PackageDependencyResolution.targets(154,5): error : Unexpected dependency 'MiniProfiler.Shared' with no version number.

@NickCraver
Copy link
Member Author

@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.

@NickCraver
Copy link
Member Author

@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 :(

@NickCraver
Copy link
Member Author

@jasselin Alrighty, now there's a very nasty workaround in place for that issue. If you try and get alpha24 or later, it should restore dependencies correctly.

Also: I've got to change the versioning here so that the alpha number isn't astronomical with CI builds.

@brdmllr
Copy link

brdmllr commented Mar 28, 2017

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!

ProviderIncompatibleException: This store command cannot be cloned because the underlying store provider does not support cloning.

@NickCraver
Copy link
Member Author

@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.

@chadwackerman
Copy link

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.

@NickCraver
Copy link
Member Author

@brdmllr I've pushed alpha29 packages to NuGet - they refactor SQL timings for all providers (without breaking the IDbProfiler interface, just simplifying it). There was old cruft from when MiniProfiler only supported SQL timings, not custom timings for anything in general. The new approach is extension methods to get custom timings, some of which are in-box (internal only). I think this is a good way to go for people to add their own custom timings in an easy, reusable, and extensible way. I'd love to hear people's thoughts. Here's an example for reference.

@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 netstandard2.0 timeframe. Still, I'd like to get them fixed ASAP.

Next up, I'll change the versioning to be something sane. The new format will be 4.0.0-alpha3-<buildNumber> - we'll be implementing this across all of the packages here at Stack Exchange. All public packages are getting public builds in Appveyor, GitHub tests integration, and MyGet feeds. Stay tuned on that front - for MiniProfiler it's a build.ps1 change and a new semver.txt to track the current release.

@NickCraver
Copy link
Member Author

For those that care, the next build will include .NET 4.5 support for v4: 4297dcf via some polyfills for AsyncLocal<T>. It's reasonable to support so...I'm doing it. Hope that helps a few people with their upgrades.

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:

  1. I'll be moving the MyGet feed to /stackoverflow (for all of our packages), not just /miniprofiler. This is for people to easily add feeds. Or I'll publish it to both, thoughts?
  2. I will be adding BenchmarkDotNet performance tests for various pieces of the profiler to we can iterate on allocations and performance.
  3. I need to port ASP.NET Core telemetry to use DiagnosticSource...I just haven't had time. And I needed the .NET 4.5 changes in before doing that AsyncLocal-style approach (or would be doing it again).

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.

@lemkepf
Copy link

lemkepf commented Apr 6, 2017

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!

@NickCraver
Copy link
Member Author

@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:/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?

@lemkepf
Copy link

lemkepf commented Apr 6, 2017

SWEET. Let me pull it down this afternoon and I'll get back to you!

@lemkepf
Copy link

lemkepf commented Apr 6, 2017

@NickCraver Two thoughts:

  1. I like the IDatabase idea. It's simple and effective for those already familiar with or already using StackExchange.Redis.
  2. A configuration overload with a connection string would be way easier though. That way the provider handles all the details about the multiplexer and the config code doesn't have to worry about it.

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....

@NickCraver
Copy link
Member Author

NickCraver commented Apr 6, 2017

@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.

@lemkepf
Copy link

lemkepf commented Apr 7, 2017

@NickCraver Yea, I can test from the MyGet feed today if it's up.

@NickCraver
Copy link
Member Author

@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

@chadwackerman
Copy link

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?

@NickCraver
Copy link
Member Author

@chadwackerman The authorization functions are on the options (per ASP.NET Core conventions) now. They still take a Func<HttpRequest, bool>, you can use them like this:

app.UseMiniProfiler(new MiniProfilerOptions
{
    RouteBasePath = "~/profiler",
    ResultsAuthorize = request => GetUser(request).CanSeeMiniProfiler,
    ResultsListAuthorize = request => GetUser(request).CanSeeMiniProfiler,
});

NickCraver added a commit that referenced this issue Apr 17, 2017
This really was missing, related to #144
@NickCraver
Copy link
Member Author

@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.

@chadwackerman
Copy link

@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.

@mattwoberts
Copy link

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?

@NickCraver
Copy link
Member Author

All,
I've started the documentation for MiniProfiler here: http://miniprofiler.com/dotnet/. It's GitHub pages built from the /docs directory (you can run it locally via Jekyll...or edit without that since it's mostly Markdown). It needs a lot more love, but it should be at least a start. I still need to document all the settings including custom providers, etc. but I wanted to get the "getting started" sections up first.

If anyone has time, can they please take a look and make suggestions?

@NickCraver
Copy link
Member Author

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

@StuffOfInterest
Copy link
Contributor

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.

@lemkepf
Copy link

lemkepf commented Oct 23, 2017

@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:

elasticSettings = elasticSettings               
	.DisableDirectStreaming()
	.OnRequestCompleted(details =>
	{
		//Get total duration
		decimal? totalDuration = (decimal?)details?.AuditTrail?.Sum(c => (c.Ended - c.Started).TotalMilliseconds);

		if (totalDuration.HasValue)
		{
			//Log to miniprofiler
			MiniProfiler.Current?.Head?.AddCustomTiming("elastic",
				new CustomTiming(MiniProfiler.Current, details.Uri.ToString())
				{
					Id = Guid.NewGuid(),
					DurationMilliseconds = (decimal?)details?.AuditTrail?.Sum(c => (c.Ended - c.Started).TotalMilliseconds),
					ExecuteType = details.HttpMethod.ToString(),
				});
		}
	});

@rahamohebbi
Copy link

@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.

@NickCraver
Copy link
Member Author

@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);
	});

@rahamohebbi
Copy link

@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 ?

@NickCraver
Copy link
Member Author

@rahamohebbi Yep, it'd be exposed :)

@NickCraver
Copy link
Member Author

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 :)

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

No branches or pull requests

9 participants