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

Add remote profile example based on httpclient #344

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ilmax
Copy link

@ilmax ilmax commented Nov 22, 2018

Add a complete end to end sample to demonstrate how to integrate a remote profiling session in MiniProfiler.
This sample uses httpclient to make a remote call and, in the remote service EF Core to query a dB, so the goal is to be able to get the all the remote profiling information and include them in the profiler tree

/cc @NickCraver

@ilmax
Copy link
Author

ilmax commented Nov 26, 2018

Hey @NickCraver did you had a chance to have a look at this?

@NickCraver
Copy link
Member

Hey @ilmax! Sorry a few crazy days here pouring concrete and prepping a big blog post. I did a cursory glance - setup looks good but I'd change how we match up the 2 ends to be much simpler. For example a .Step() returns the Timing in 4.0+, this means you could take that step and set the ID to the header, rather than setting the step name and parsing it later. It's okay for these GUIDs to match (different tables) and makes matching both more efficient and much easier to do in aggregate, say from a SQL table or any storage mechanism a user may be utilizing :)

@ilmax
Copy link
Author

ilmax commented Nov 27, 2018

Hey @NickCraver, no problem :) Looking forward to the post!
I'm not sure I understood you correctly, do you mean to change the code before the remote call to use the id of the step wrapping the remote call, to store some information like the remote profiling id and the remote host into a storage mechanism (e.g. a dictionary) to avoid the "match by magic name" thing and subsequent parsing?

@NickCraver
Copy link
Member

For example, instead of:

MiniProfiler.Current.Step($"RemoteStep:{request.RequestUri.GetLeftPart(UriPartial.Authority)}:{s.Single()}")

You can...

var step = MiniProfiler.Current.Step("RemoteStep: " + request.RequestUri.GetLeftPart(UriPartial.Authority))
step.Id = Guid.Parse(s.Single());

...then in the matching, we're not doing any string parsing :)

@ilmax
Copy link
Author

ilmax commented Nov 28, 2018

@NickCraver I've changed it to follow your guideline, I've left the remote host in the step name, because it might be useful when you do remote calls to multiple hosts.

Let me know if this looks good to you!

@NickCraver
Copy link
Member

Alrighty, finally got a chance to load this up tonight. I think we can simplify here greatly, and trim the example way down.

In short: though we want to call a remote API, for the purposes of the example there need not be 2 apps...one app can call itself (at a different relative path). It also simplified the fetch of the code because it can be from the same store as intended. In production this would be Redis or SQL or whatever, but here it's in-memory and the example holds (we can just add comments there). The single app example also makes it much easier for people to see in action, e.g. F5 launches from Visual Studio work.

I'm happy to make the simplifications here, but also don't want to step on toes - what do you think?

@ilmax
Copy link
Author

ilmax commented Dec 1, 2018

Hey @NickCraver, I think going to single application approach it's a good idea! You can take over this or I can do it after the weekend. Whatever you like!

@NickCraver NickCraver changed the base branch from master to main June 20, 2020 13:21
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.

2 participants