Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Support scenario when IsEnabled is called to control flow but there is no event for the same diagnosticName #67

Closed
lmolkova opened this issue Apr 22, 2017 · 3 comments

Comments

@lmolkova
Copy link

lmolkova commented Apr 22, 2017

We have recently introduced a new DiagnosticSource usage pattern in the HttpClient and AspNetCore.Hosting:

E.g. in AspNetCore.Hosting:

  1. producer calls IsEnabled("Microsoft.AspNetCore.Hosting.HttpRequestIn", httpContext) just to determine that listener wants instrumentation to happen. Producer optionally adds context that could help consumer to check if he wants instrumentation for this particular request.
  2. if 1 is true, producer calls IsEnabled("Microsoft.AspNetCore.Hosting.HttpRequestIn.Start") to check if consumer wants start event and if true, writes event with the same name
  3. producers just writes IsEnabled("Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop") if instrumentation was on in step 1.

The problem that even if there is no event for the first IsEnabled, user still has to write a method with corresponding DiagnosticName attribute.

    class HttpHandlerListener
    {
        [DiagnosticName("System.Net.Http.HttpRequestOut")]
        public void IsEnabled()
        {
        }

        [DiagnosticName("System.Net.Http.HttpRequestOut.Start")]
        public void OnStart(HttpRequestMessage request)
        {
        }

        [DiagnosticName("System.Net.Http.HttpRequestOut.Stop")]
        public void Stop(HttpResponseMessage response)
        {
        }
    }

I have tried to work it around in the other way with SubscribeWithAdapter(object, Predicate isEnabled),
but isEnabled never gets called if there is no attribute with this diagnosticName.

As a minimum solution, I would expect isEnabled predicate (provided to SubscribeWithAdapter) at least be called and it's result should be taken into account.

But may be IsEnabled should be done in the same way as Write?

[DiagnosticName("System.Net.Http.HttpRequestOut")] //or another attribute?
public bool IsEnabled(HttpContext context)
{
   return context.Request.Method != "OPTIONS";
}

And if there is no IsEnabled, DiagnosticName on the Write callback could be used as an indicator that event is enabled

@Eilon
Copy link
Member

Eilon commented Jun 6, 2017

@rynowak thoughts?

@Eilon Eilon added this to the Backlog milestone Jul 5, 2017
@Eilon
Copy link
Member

Eilon commented Jul 5, 2017

Marking as up-for-grabs and moving to Backlog. This seems like a reasonable feature request, but we're not investing a lot in the automatic proxy generation logic at this time.

@aspnet-hello
Copy link

This issue was moved to aspnet/Home#2325

@aspnet aspnet locked and limited conversation to collaborators Jan 1, 2018
@aspnet-hello aspnet-hello removed this from the Backlog milestone Jan 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants