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 support for hooking in additional instrumentation around Timings #650

Merged
merged 4 commits into from
May 23, 2023

Conversation

m0sa
Copy link
Contributor

@m0sa m0sa commented May 23, 2023

Most of the code of a project that I'm working on is instrumented via MiniProfiler's extension methods. In order to get the MiniProfiler instrumented sections/traces into 3rd party APM tools, we'd either need to hijack all the MiniProfiler extension method combinations, or add yet another using around everything we instrument via MiniProfiler. The latter is particularly impractical for the .Inline(() => ....) extension methods etc.

This is just a little something to get a discussion going. How opposed would you be to something like this?

@NickCraver
Copy link
Member

Hey there! I'm not opposed to this if we make it opt-in. Even if we don't have an activity source, we eat 2x the creation time at a base level for Timing, ran some quick benchmarks:

Method Mean Error StdDev Gen0 Allocated
ActivityNoListener 10.35 ns 2.243 ns 0.123 ns - -
ActivityWithListener 84.75 ns 53.480 ns 2.931 ns 0.0119 200 B
TimingNew 12.09 ns 21.175 ns 1.161 ns 0.0134 224 B
TimingWithNoListener 25.08 ns 47.574 ns 2.608 ns 0.0134 224 B
void Main()
{
    Util.AutoScrollResults = true;
    BenchmarkRunner.Run<ActivityBenchmarks>();
}

[ShortRunJob]
[MemoryDiagnoser]
public class ActivityBenchmarks
{
    private static ActivitySource _noListener = new ActivitySource("NoListener.MiniProfiler.Timing", "1.0.0.0");
    private static ActivitySource _listener = new ActivitySource("Listener.MiniProfiler.Timing", "1.0.0.0");

    [GlobalSetup]
    public void Setup()
    {
        ActivitySource.AddActivityListener(new ActivityListener()
        {
            ShouldListenTo = s => s.Name == "Listener.MiniProfiler.Timing",
            Sample = (ref ActivityCreationOptions<ActivityContext> _) => ActivitySamplingResult.AllData,
        });
    }

    [Benchmark]
    public Activity ActivityNoListener() => _noListener.CreateActivity("Test", ActivityKind.Internal);
    [Benchmark]
    public Activity ActivityWithListener() => _listener.CreateActivity("Test", ActivityKind.Internal);

    [Benchmark]
    public Timing TimingNew() => new Timing();

    [Benchmark]
    public Timing TimingWithNoListener()
    {
        var a = _noListener.CreateActivity("Test", ActivityKind.Internal);
        GC.KeepAlive(a);
        return new Timing();
    }
}

Thoughts about making it cheaper for everyone else not opting in? (also: please sanity check quick benchmarks above)

@m0sa
Copy link
Contributor Author

m0sa commented May 23, 2023

@NickCraver we don't use ActivitySource / Listener by default either, and doing having to use a Listener just to forward it to something else is indeed an overkill. I think this more minimal approach would be more flexible for folks to hook in whatever they need, while also being more lightweight.

@m0sa m0sa changed the title Add activity tracing via Timing class Add support for hooking in additional instrumentation around Timings May 23, 2023
@NickCraver
Copy link
Member

@m0sa I dig it, because later this could be a one-liner to set to a static Func that could hook Activity the same way and we could bundle that in-box too if no additional dependencies. Let's get this in :)

@m0sa m0sa marked this pull request as ready for review May 23, 2023 18:11
@NickCraver NickCraver merged commit 1120572 into MiniProfiler:main May 23, 2023
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