Prometheus fix metrics #23

tiagodaraujo · 2024-04-08T09:54:33Z

Description

First Commit - feat: normal priority as default task execution

Changed the default Execute TaskItem default priority from Critical(0) to Normal.

Second Commit - fix: prometeus Inc/Dec gauge metrics

In this pull #19, I tried to remove the counter copies to use the counter reference and then set the Prometheus gauge value with the counter instant value. I tested in production and the issue persists.

The Prometheus documentation suggests using the Inc and Dec method for a queue problem as we have here.

As I mentioned in #22, it is possible to replicate using only unit tests.

This commit changes the use of gauge.Set to gauge.Inc and gauge.Dec in the queue and concurrency events.

Thrid Commit - fix: prometheus resolves UNKNOWN method

After the second commit, I improved the integration tests to add more tests with more than one priority task and added a second HTTP Method (Delete).

In this test, is possible to watch the IAdaptativeLimiterOptionsExtensions.GetMethod losing the HttpContext and resolving the UNKNOWN method.

I propose to send the method on ITaskManager.AcquireAsync used by Middleware and then we never need to resolve the method anymore during the task execution. Additionally allows users to define the method they want outside of the use of AdaptiveConcurrencyLimiterMiddleware.

Fixes #22

How Has This Been Tested?

Run the integration tests

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have added tests to cover my changes
I have made corresponding changes to the documentation

Disclaimer

By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement

) Bumps [express](https://github.com/expressjs/express) from 4.18.2 to 4.19.2. - [Release notes](https://github.com/expressjs/express/releases) - [Changelog](https://github.com/expressjs/express/blob/master/History.md) - [Commits](expressjs/express@4.18.2...4.19.2) --- updated-dependencies: - dependency-name: express dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

tiagodaraujo · 2024-04-08T10:00:38Z

src/Farfetch.LoadShedding/Limiters/AdaptativeConcurrencyLimiter.cs

        {
-            return this.ExecuteAsync(0, function, cancellationToken);
+            return this.ExecuteAsync(Priority.Normal, function, method, cancellationToken);


question for maintainers: Regarding the first commit, should the default priority be Normal or should we keep it as a Critical(0) priority?

The default priority should be normal.

tiagodaraujo · 2024-04-08T10:06:57Z

src/Farfetch.LoadShedding.Prometheus/Metrics/HttpRequestsConcurrencyItemsGauge.cs

+        public void Increment(string method, string priority)
+        {
+            this.Metric?
+                .WithLabels(method, priority)


question for maintainers, I didn't see any use of the method in this framework or even in the Grafana dashboard.

What are the plans for this type of custom labels?

My implementation proposal may be slightly misaligned depending on the vision for these labels.

On the one hand, requiring the method in a base interface can compromise the coupling. On the other hand, leaving Prometheus to resolve the method closes the to a HtppClient implementation, leaving out other clients and background tasks.

Yes, I would say that the http method is useless for now and maybe we can remove that instead of creating a coupling.

tiagodaraujo · 2024-04-08T10:17:42Z

Benchmark results

MAIN is the main branch, FIX-QUEUE includes this pull request and the previous changes published in the beta version 1.0.1-beta.

| Method                           | Mean     | Error   | StdDev  | Min      | Max      | Rank | Gen0   | Completed Work Items | Lock Contentions | Allocated |
|--------------------------------- |---------:|--------:|--------:|---------:|---------:|-----:|-------:|---------------------:|-----------------:|----------:|
// MAIN
| Limiter_Default                  | 279.9 ns | 3.21 ns | 2.12 ns | 274.8 ns | 282.4 ns |    1 | 0.0572 |               0.0000 |                - |     720 B |
| Limiter_RandomPriority           | 293.1 ns | 4.92 ns | 2.93 ns | 289.6 ns | 299.3 ns |    2 | 0.0572 |               0.0000 |                - |     720 B |
| LimiterMiddleware_Default        | 319.2 ns | 3.07 ns | 1.83 ns | 316.4 ns | 322.4 ns |    3 | 0.0648 |               0.0000 |                - |     816 B |
| LimiterMiddleware_RandomPriority | 322.8 ns | 3.56 ns | 2.35 ns | 319.3 ns | 327.5 ns |    3 | 0.0687 |               0.0000 |                - |     864 B |
// FIX QUEUE
| Limiter_Default                  | 226.8 ns | 10.25 ns |  6.78 ns | 217.6 ns | 236.5 ns |    2 | 0.0484 |               0.0000 |                - |     608 B |
| Limiter_RandomPriority           | 217.8 ns |  9.43 ns |  5.61 ns | 208.8 ns | 223.7 ns |    1 | 0.0484 |               0.0000 |                - |     608 B |
| LimiterMiddleware_Default        | 250.1 ns | 22.37 ns | 14.80 ns | 237.3 ns | 275.5 ns |    3 | 0.0558 |               0.0000 |                - |     704 B |
| LimiterMiddleware_RandomPriority | 265.5 ns | 17.12 ns | 11.33 ns | 249.9 ns | 285.1 ns |    4 | 0.0596 |               0.0000 |                - |     752 B |
// DIFF
CPU -21%
Memory -14%

| Method                                       | Mean     | Error     | StdDev    | Min      | Max       | Rank | Completed Work Items | Lock Contentions | Allocated |
|--------------------------------------------- |---------:|----------:|----------:|---------:|----------:|-----:|---------------------:|-----------------:|----------:|
// MASTER
| TaskQueueWith1000Items_EnqueueFixedPriority  | 1.538 us | 0.5865 us | 0.3068 us | 1.200 us |  2.000 us |    1 |                    - |                - |     592 B |
| TaskQueueEmpty_EnqueueRandomPriority         | 2.267 us | 1.2715 us | 0.7566 us | 1.300 us |  3.400 us |    2 |                    - |                - |     592 B |
| TaskQueueWith1000Items_EnqueueRandomPriority | 1.970 us | 0.7669 us | 0.5073 us | 1.350 us |  2.650 us |    2 |                    - |                - |     592 B |
| TaskQueueWith1000Items_Dequeue               | 2.720 us | 1.8568 us | 1.2282 us | 1.800 us |  5.300 us |    2 |                    - |                - |     544 B |
| TaskQueue_EnqueueNewItem_LimitReached        | 7.460 us | 3.5054 us | 2.3186 us | 3.900 us | 10.800 us |    3 |                    - |                - |     736 B |
// FIX QUEUE
| TaskQueueWith1000Items_EnqueueFixedPriority  | 1.670 us | 0.7477 us | 0.4945 us | 0.9000 us | 2.400 us |    1 |                    - |                - |     600 B |
| TaskQueueEmpty_EnqueueRandomPriority         | 1.222 us | 0.5095 us | 0.3032 us | 0.9000 us | 1.900 us |    1 |                    - |                - |     600 B |
| TaskQueueWith1000Items_EnqueueRandomPriority | 2.070 us | 0.5922 us | 0.3917 us | 1.3000 us | 2.500 us |    2 |                    - |                - |     312 B |
| TaskQueueWith1000Items_Dequeue               | 2.156 us | 0.5187 us | 0.3087 us | 1.8000 us | 2.800 us |    2 |                    - |                - |     472 B |
| TaskQueue_EnqueueNewItem_LimitReached        | 2.600 us | 1.3729 us | 0.8170 us | 2.0000 us | 4.500 us |    2 |                    - |                - |     600 B |
// DIFF
CPU -39%
Memory -15%

brmagadutra · 2024-04-09T07:47:24Z

src/Farfetch.LoadShedding.Prometheus/Metrics/HttpRequestsConcurrencyItemsGauge.cs

        /// </summary>
        /// <param name="method">The method.</param>
        /// <param name="priority">The priority.</param>
-        /// <param name="value">The value.</param>
-        public void Set(string method, string priority, double value)
+        public void Decrement(string method, string priority)


This change introduces a breaking change, if, for some reason, someone calls the Set method, right?

The same applies to other changes from a Set method to an Increment and Decrement methods.

Hi @brmagadutra , The class and method are public, so I would say, Yes, it's a breaking change. However, I think everyone should call the extension IAdaptativeLimiterOptionsExtensions.AddMetrics to configure the LoadSheddingOptions. It will have no impact on who uses this extension method that configures the gauges metrics.

For example, internally we, Farfetch, use always the Options extension.

I understand, and this would be the best way. Still, we have this scenario to consider, so if the rest of the team is ok I won't oppose going forward. @joelfoliveira and @ailtonguitar what do you think?

Yes, It is a breaking change but I think that the impact is very low.
But what is the rational of this change?

ailtonguitar · 2024-04-15T13:39:15Z

src/Farfetch.LoadShedding/Limiters/IAdaptativeConcurrencyLimiter.cs

        /// <param name="cancellationToken">A cancellation token is used to signal that the running operation should be stopped.</param>
        /// <returns></returns>
-        Task ExecuteAsync(Priority priority, Func<Task> function, CancellationToken cancellationToken = default);
+        Task ExecuteAsync(Priority priority, Func<Task> function, string method = null, CancellationToken cancellationToken = default);


The method should not be passed here, it is the base lib and it can be used in other context (it is not exclusive for web requests)

ailtonguitar · 2024-04-15T13:39:42Z

src/Farfetch.LoadShedding/Tasks/ITaskItem.cs

+{
+    public interface ITaskItem
+    {
+        string Method { get; }


The base lib should not be coupled to web implementation

ailtonguitar · 2024-04-15T13:40:43Z

src/Farfetch.LoadShedding.Prometheus/IAdaptativeLimiterOptionsExtensions.cs

@@ -65,18 +61,17 @@ public static class IAdaptativeLimiterOptionsExtensions

            events.ItemDequeued.Subscribe(args =>
            {
-                var method = accessor.GetMethod();


What is the rational of removing the method resolver from the accessor?

* feat: grafana dashboard V2 * fix: decrease queue once on reject * fix: queue gauge metric * chore: remove xUnit traits from Benchmark and Performance projects

This reverts commit 8b4b690.

tiagodaraujo mentioned this pull request Apr 8, 2024

[Bug Report]: http_requests_queue_items_total is not zero after process entire queue #22

Closed

1 task

tiagodaraujo marked this pull request as ready for review April 8, 2024 09:57

tiagodaraujo commented Apr 8, 2024

View reviewed changes

brmagadutra approved these changes Apr 9, 2024

View reviewed changes

ruiqbarbosa approved these changes Apr 9, 2024

View reviewed changes

JoaoRodriguesGithub approved these changes Apr 10, 2024

View reviewed changes

ailtonguitar reviewed Apr 15, 2024

View reviewed changes

fasazevedo approved these changes Apr 15, 2024

View reviewed changes

ailtonguitar and others added 10 commits April 22, 2024 15:49

fix: loadshedding hidding errors

04db12b

chore: improve unit test for exception handing

f7ecbd1

chore: unit test refactoring

7cdb1a9

fix: default priority

810350e

fix: unknown http method issue

7f017cf

Fix queue (Farfetch#19)

ac4b820

* feat: grafana dashboard V2 * fix: decrease queue once on reject * fix: queue gauge metric * chore: remove xUnit traits from Benchmark and Performance projects

fix: prometeus Inc/Dec gauge metrics

94ca3be

fix: prometheus resolves UNKNOWN method

3a132bf

Revert "fix: prometheus resolves UNKNOWN method"

021d809

This reverts commit 8b4b690.

SAVEPOINT

e0bd8ac

tiagodaraujo force-pushed the fix-queue branch from 8b4b690 to e0bd8ac Compare May 7, 2024 14:32

tiagodaraujo closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prometheus fix metrics #23

Prometheus fix metrics #23

tiagodaraujo commented Apr 8, 2024 •

edited

Loading

tiagodaraujo Apr 8, 2024

ailtonguitar Apr 15, 2024

tiagodaraujo Apr 8, 2024 •

edited

Loading

ailtonguitar Apr 15, 2024

tiagodaraujo commented Apr 8, 2024

brmagadutra Apr 9, 2024

tiagodaraujo Apr 9, 2024 •

edited

Loading

brmagadutra Apr 9, 2024

ailtonguitar Apr 15, 2024

ailtonguitar Apr 15, 2024

ailtonguitar Apr 15, 2024

ailtonguitar Apr 15, 2024

Prometheus fix metrics #23

Prometheus fix metrics #23

Conversation

tiagodaraujo commented Apr 8, 2024 • edited Loading

Description

First Commit - feat: normal priority as default task execution

Second Commit - fix: prometeus Inc/Dec gauge metrics

Thrid Commit - fix: prometheus resolves UNKNOWN method

How Has This Been Tested?

Checklist

Disclaimer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagodaraujo Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagodaraujo commented Apr 8, 2024

Benchmark results

Choose a reason for hiding this comment

tiagodaraujo Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagodaraujo commented Apr 8, 2024 •

edited

Loading

tiagodaraujo Apr 8, 2024 •

edited

Loading

tiagodaraujo Apr 9, 2024 •

edited

Loading