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

Modify EventCounter usage to support the new metric APIs as well #33387

Closed
12 tasks done
Tracked by #79371
noahfalk opened this issue Jun 9, 2021 · 20 comments
Closed
12 tasks done
Tracked by #79371

Modify EventCounter usage to support the new metric APIs as well #33387

noahfalk opened this issue Jun 9, 2021 · 20 comments
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Milestone

Comments

@noahfalk
Copy link
Member

noahfalk commented Jun 9, 2021

Tasks:


I've been experimenting with how a library could optionally support the new Meter APIs while also being back-compatible with pre-existing EventCounters. It would be nice to test this on a more real-world use case in ASP.Net to see if we like how it plays out. If it doesn't look good of course we could always change the pattern.

The rough pattern I had was to:

  1. Add the new Meter API usage SxS with the EventCounters.
  2. Switch code that was directly invoking into EventCounter.WriteMetric + IncrementingEventCounter.WriteMetric so that it instead calls Counter.Add() or Histogram.Record().
  3. Add a forwarder so that those Counter.Add() + Histogram.Record() calls with also chain to invoking the pre-existing WriteMetric().
  4. For PollingCounter + IncrementingPollingCounter, pull the data from the same API that ObservableCounter/ObservableGauge pull it from.

In total it looked like this...
New Meter code + app logic:

using System;
using System.Collections.Generic;
using System.Diagnostics.Metrics;
using System.Threading;
using System.Threading.Tasks;

namespace ConsoleApp
{
    class Program
    {
        static Meter s_meter = new Meter("ContusoHatStore");
        internal static Counter<int> s_hatsSold = 
            s_meter.CreateCounter<int>("Hats-Sold", "Hats", "Number of Hats Sold");
        internal static ObservableCounter<long> s_transactionsProcessed = 
            s_meter.CreateObservableCounter<long>("TransactionsProcessed", GetTransactions, "Transactions", "Number of Transactions queued");
        internal static ObservableGauge<long> s_heapSize = 
            s_meter.CreateObservableGauge<long>("Heap-Size", GetHeapSize,
            "B", "Size of GC heap");
        internal static Histogram<double> s_histogram = 
            s_meter.CreateHistogram<double>("Request-Latency", "ms", "Request Latencies");

        static long s_transactions;
        internal static long GetHeapSize() => GC.GetTotalMemory(false);
        internal static long GetTransactions() => s_transactions;

        static async Task Main(string[] args)
        {
            ContusoHatStoreEventSource contusoHatStoreEventSource = new ContusoHatStoreEventSource();

            CancellationTokenSource cts = new CancellationTokenSource();
            Task t = ProcessPretendTransactions(cts.Token);
            Console.WriteLine("Transactions running, hit enter to stop");
            Console.ReadLine();
            cts.Cancel();
            await t;
        }

        static async Task ProcessPretendTransactions(CancellationToken token)
        {
            while(!token.IsCancellationRequested)
            {
                PretendTransaction();
                await Task.Delay(10);
            }
        }

        static void PretendTransaction()
        {
            Random r = new Random();
            s_hatsSold.Add(r.Next(1, 5), KeyValuePair.Create<string,object>("Size", r.Next(3,10)), KeyValuePair.Create<string,object>("Color", r.Next(1000) > 500 ? "Blue" : "Red"));
            s_transactions++;
            s_histogram.Record(r.Next(100, 500));
        }
    }
}

Pre-existing EventCounter code + forwarder:

using System;
using System.Collections.Generic;
using System.Diagnostics.Metrics;
using System.Diagnostics.Tracing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApp
{
    [EventSource(Name ="ContusoHatStore")]
    class ContusoHatStoreEventSource : EventSource
    {
        IncrementingEventCounter m_hatsSold;
        IncrementingPollingCounter m_transactionsProcessed;
        PollingCounter m_heapSize;
        EventCounter m_requestLatencies;
        InstrumentForwarder _forwarder = new InstrumentForwarder();

        public ContusoHatStoreEventSource() : base("ContusoHatStore")
        {
            m_hatsSold = new IncrementingEventCounter("Hats-Sold", this)
            {
                DisplayName = "Hats Sold",
                DisplayUnits = "Hats"
            };
            m_transactionsProcessed = new IncrementingPollingCounter("Transactions", this, () => Program.GetTransactions())
            {
                DisplayName = "Transactions",
                DisplayUnits = "Transactions"
            };
            m_heapSize = new PollingCounter("Heap-Size", this, () => Program.GetHeapSize())
            {
                DisplayName = "Heap Size",
                DisplayUnits = "B"
            };
            m_requestLatencies = new EventCounter("Request-Latency", this)
            {
                DisplayName = "Request Latency",
                DisplayUnits = "ms"
            };

            _forwarder.Forward(Program.s_hatsSold, value => m_hatsSold.Increment(value));
            _forwarder.Forward(Program.s_histogram, value => m_requestLatencies.WriteMetric(value));
        }
    }

    class InstrumentForwarder
    {
        MeterListener _listener = new MeterListener();
        public void Forward<T>(Counter<T> counter, Action<T> action) where T : struct
        {
            _listener.SetMeasurementEventCallback<T>((instrument, value, tags, state) => { ((Action<T>)state)(value); });
            _listener.EnableMeasurementEvents(counter, action);
        }
        public void Forward<T>(Histogram<T> histogram, Action<T> action) where T : struct
        {
            _listener.SetMeasurementEventCallback<T>((instrument, value, tags, state) => { ((Action<T>)state)(value); });
            _listener.EnableMeasurementEvents(histogram, action);
        }
    }
}

@shirhatti

@shirhatti
Copy link
Contributor

shirhatti commented Jun 9, 2021

I'll start with Hosting, but if this pattern works well, we have a few more metrics sources to address

  • HostingEventSource
  • ConcurrencyLimiterEventSource
  • KestrelEventSource
  • HttpConnectionsEventSource
  • Rate limiting

@BrennanConroy BrennanConroy added this to the Next sprint planning milestone Jun 9, 2021
@ghost
Copy link

ghost commented Jun 9, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@shirhatti
Copy link
Contributor

@davidfowl

@adityamandaleeka
Copy link
Member

@shirhatti Can you update this issue with current state and re-milestone it as needed?

@shirhatti
Copy link
Contributor

@ghost
Copy link

ghost commented Sep 28, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@noahfalk
Copy link
Member Author

I forgot we had this issue. The current path I want to explore is if we can have the runtime automatically create a Meter with the same name for any EventSource that creates counters. If that path was successful ASP.Net wouldn't need to do any specific work until you wanted new features not available with EventCounters (such as histograms for latency percentiles).

@samsp-msft
Copy link
Member

@davidfowl @adityamandaleeka @noahfalk @tommcdon - from my initial spelunking of this repo, I was expecting to see that ASP.NET Core would be extensively using Meters, but from what I can tell, it's not using it yet?
For example I would expect to see the counters for

[Microsoft.AspNetCore.Hosting]
    Current Requests                                                       0
    Failed Requests                                                        0
    Request Rate (Count / 1 sec)                                           0
    Total Requests                                                        45

to be updated to be dimensioned histogram meters based on the path that is being used?
Although that does bring up interesting questions as to what the dimensions should be?
HTTP Method, and Path would be logical, but path gets complicated when you have to account for parameterization, as you typically want to limit the number of parameters and values used.

Added #79371 to consider where we want the BCL to support meters

@davidfowl
Copy link
Member

I think we want very simple meters with very simple dimensions in the core framework. Users can make their own meters if they need lots of other dimensions.

@adityamandaleeka
Copy link
Member

See #46834

@adityamandaleeka
Copy link
Member

@JamesNK Is this done now after #46834 or is there more to do for this issue?

@JamesNK
Copy link
Member

JamesNK commented Apr 18, 2023

There is also #47758 (rate limiting) and #47618 (replace hack with Microsoft.Extensions.Metrics)

I'll update this issue with a list of tasks.

@adityamandaleeka
Copy link
Member

Got it. I didn't realize this was being used as the meta-issue.

@captainsafia captainsafia added the blocked The work on this issue is blocked due to some dependency label May 9, 2023
@captainsafia
Copy link
Member

Currently blocked on the completion of #47618.

@adityamandaleeka
Copy link
Member

@JamesNK Is this done now?

@JamesNK
Copy link
Member

JamesNK commented May 31, 2023

Added a couple of work items to do in .NET 8.

@JamesNK Is this done now?

It will never be done 😈

@JamesNK JamesNK removed the blocked The work on this issue is blocked due to some dependency label May 31, 2023
@adityamandaleeka
Copy link
Member

@JamesNK Is the rest of this still targeting .NET 8?

@adityamandaleeka adityamandaleeka modified the milestones: 8.0-preview5, 8.0-rc1 Aug 1, 2023
@JamesNK
Copy link
Member

JamesNK commented Aug 1, 2023

Won't have time for AuthN/AuthZ counters. Everything else will happen in .NET 8.

@joperezr
Copy link
Member

joperezr commented Aug 17, 2023

Now that you've merged the docs PR, can this be closed @JamesNK ? (since we already have a separate issue to track AuthN/AuthZ)

@JamesNK JamesNK closed this as completed Aug 17, 2023
@adityamandaleeka
Copy link
Member

Feels weird seeing this issue closed. End of an era. 😆

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

No branches or pull requests