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

Changing handler to work with singletons #10

Merged
merged 1 commit into from
Aug 11, 2020
Merged

Conversation

cvallance
Copy link

Given that the recommended lifetime of an ElasticClient is for it to be a singleton, I've changed / updated the handler to work with this.

Note: Sorry, I'm on linux and the whole project wouldn't build so I just took a stab at what sould be removed / addded etc.

@romansp
Copy link
Owner

romansp commented Jul 30, 2020

Hi @cvallance, thanks for bringing my attention to this and I'm more than willing to merge if it addresses the issue you're experiencing. Unfortunately I haven't used neither MiniProfiler nor ElasticClient in a long time. I will try to look into it this week and hopefully release a fixed nuget package.

Regarding solution compilation I believe it's legacy ASP.NET MVC sample that won't build? Will do something about it too.

@cvallance
Copy link
Author

Thanks @romansp . That would be great if you can fix it but no rush... I've just taken your code and put it into my client creation as is. So I can tell you that this works:

settings.OnRequestCompleted(details =>
{
    var profiler = MiniProfiler.Current;
    if (profiler?.Head == null || details?.DebugInformation == null)
    {
        return;
    }

    var duration = details.AuditTrail?.Sum(c => (c.Ended - c.Started).TotalMilliseconds) ?? 0;
    profiler.Head.AddCustomTiming("elasticsearch",
        new CustomTiming(profiler, details.DebugInformation)
        {
            Id = Guid.NewGuid(),
            DurationMilliseconds = Convert.ToDecimal(duration),
            ExecuteType = details.HttpMethod.ToString(),
        });
});

_client = new ElasticClient(settings);

@romansp
Copy link
Owner

romansp commented Jul 30, 2020

Oh, okay great!

Does it mean that previous implementation could be possibly causing a memory-leak by pinning reference to MiniProfiler instance inside _profiler field?

@cvallance
Copy link
Author

cvallance commented Jul 30, 2020

Totally depends on the lifecycle management of the ElasticClient instance I guess. I think it would've only been a leak if they had a singleton instance and even then it's only 1 instance... plus it wouldn't have worked as I found out.

End of the day, no big deal 😄

romansp added a commit that referenced this pull request Aug 11, 2020
migrate sample to dotnet core 3.1
register IElasticClient as singleton to demonstrate #10
@romansp romansp mentioned this pull request Aug 11, 2020
@romansp romansp merged commit e7c239c into romansp:master Aug 11, 2020
@romansp
Copy link
Owner

romansp commented Aug 11, 2020

Hey @cvallance! Sorry for the delay, 6.1.1 package is up on the nuget feed. Please let me know of any issues. And thanks again for your fix.

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