[prometheus] Fixed a race condition happening in simultaneous OpenMetrics and PlainText requests

[hez2010]

Fixes #5515

Changes

Fixed a race condition which can cause returning an incorrect response while OpenMetrics requests and PlainText requests are being processed simultaneously.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: hez2010 / name: Steve (7df9d9c, 224d1bd, 1c843d0, 2804a7d, 00149ef, 0e2551a, c1af823, fd01eb2, 19ae570, c2e10df, 286d23d, 3ee6067, cb95766, 0269b93, 45dadb2, 580257e, 677b121, 7ee3712, 63f32d6, 3e003f8, fb54fc8, d930f97, 3558e9f, aaf7e43, 6e5549f, aed502c)
• ✅ login: CodeBlanch / name: Mikel Blanchard (017dd39, 7b3b31e)
• ✅ login: vishweshbankwar / name: Vishwesh Bankwar (033148c, 90c348c)
• ✅ login: reyang / name: Reiley Yang (665e390)

[codecov]

Codecov Report

Attention: Patch coverage is 71.64179% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 85.49%. Comparing base (6250307) to head (665e390).
Report is 222 commits behind head on main.

❗ Current head 665e390 differs from pull request most recent head aed502c. Consider uploading reports for the commit aed502c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5517      +/-   ##
==========================================
+ Coverage   83.38%   85.49%   +2.11%     
==========================================
  Files         297      289       -8     
  Lines       12531    12515      -16     
==========================================
+ Hits        10449    10700     +251     
+ Misses       2082     1815     -267     

Flag	Coverage Δ
unittests	?
unittests-Solution-Experimental	85.25% <71.64%> (?)
unittests-Solution-Stable	85.45% <71.64%> (?)
unittests-Unstable-Core	20.01% <71.64%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...metheus.AspNetCore/PrometheusExporterMiddleware.cs	70.00% <100.00%> (ø)
....Prometheus.HttpListener/PrometheusHttpListener.cs	81.92% <100.00%> (+0.22%)	⬆️
...tpListener/Internal/PrometheusCollectionManager.cs	75.67% <67.79%> (-2.11%)	⬇️

... and 78 files with indirect coverage changes

[hez2010]

The race condition is still present and need more work. I'm converting it to draft for now.

[reyang]

Thank you for looking into this @hez2010!
Adding @robertcoltheart as FYI.

I think we need some architecture change here. The original design caches the response with the assumption that the same response payload can be used for all scrapers. With the ability to support both Prometheus (text/plain; charset=utf-8; version=0.0.4) and OpenMetrics (application/openmetrics-text; version=1.0.0; charset=utf-8) format, I guess we need to have separate cache for each potential response body, or we don't cache at all.

[hez2010]

Now it's ready for review.
/cc: @reyang @robertcoltheart @vishweshbankwar @CodeBlanch

[reyang]

There are some minor issues regarding markdownlint and line-ending, LGTM once those are fixed.

[hez2010]

I pushed a new commit to simplify the code, and resolved another race condition due to the bad exchange of this.collectionTcs.
The test PrometheusExporterMiddlewareIntegration_MultipleRequestsOfDifferentFormatsInParallel now passed with even 100,000,000 requests on my machine.

[vishweshbankwar]

I pushed a new commit to simplify the code, and resolved another race condition due to the bad exchange of this.collectionTcs. The test PrometheusExporterMiddlewareIntegration_MultipleRequestsOfDifferentFormatsInParallel now passed with even 100,000,000 requests on my machine.

@hez2010 - Could you elaborate what was the race condition?

[hez2010]

@hez2010 - Could you elaborate what was the race condition?

We are neither using lock (obj) nor inserting a memory barrier around this.collectionTcs = ..., so it can sometimes result in accessing an invalid collectionTcs by another thread due to memory order issues.
Here I changed it to use CompareExchange instead.

You can observe the issue by running the test I added (and better to increase the request times). Without the change, the test sometimes never finishes.

[vishweshbankwar]

@hez2010 - Could you elaborate what was the race condition?

We are neither using lock (obj) nor inserting a memory barrier around this.collectionTcs = ..., so it can sometimes result in accessing an invalid collectionTcs by another thread due to memory order issues. Here I changed it to use CompareExchange instead.

You can observe the issue by running the test I added (and better to increase the request times). Without the change, the test sometimes never finishes.

This issue would surface even without any of these changes? Meaning, if we just run the test with single value passed here

[hez2010]

Seems that I accidentally broke the cache. Working on a fix.

[hez2010]

It turns out to be an invalid assert in the test.

[hez2010]

This issue would surface even without any of these changes? Meaning, if we just run the test with single value passed here

Yes. My newly added test sometimes never finishes against the existing code as well.

[hez2010]

All things should work expected now. PTAL.

[vishweshbankwar]

This issue would surface even without any of these changes? Meaning, if we just run the test with single value passed here

Yes. My newly added test sometimes never finishes against the existing code as well.

Thanks for confirming @hez2010. Looks like this PR is now trying to solve two issues.

1. Issue of supporting two formats simultaneously.
2. Concurrency issue.

Here is what I think we should do.

1. Keep this PR simple and solve the issue of not supporting two formats simultaneously. The original change you had by simply adding two buffers.

2. Propose a separate change for fixing concurrency issue. A detailed issue with a simple test first would be helpful.

[hez2010]

I can separate this PR into two parts as you mentioned, but the tests will gonna fail without the fix for the concurrency issue (I think you may have noticed that some previous CI run timeouts because they failed to complete the test due to this) so that the PR will not be a good state that can be merged IMO.

[vishweshbankwar]

I can separate this PR into two parts as you mentioned, but the tests will gonna fail without the fix for the concurrency issue (I think you may have noticed that some previous CI run timeouts because they failed to complete the test due to this) so that the PR will not be a good state that can be merged IMO.

For the test, you can change it to sequential execution for now. The test should validate that the exporter can serve both the formats. When working on concurrency issue, can add the parallel test back.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[vishweshbankwar]

@hez2010 - Checking to see if you are planning to do the changes based on the discussion above?

[hez2010]

@hez2010 - Checking to see if you are planning to do the changes based on the discussion above?

Thanks for pinging!

Sorry for the delay. I have been busy recently so may not be able to apply the changes right now.
While feel free to pick up the PR if it's urgent.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[robertcoltheart]

I can probably pick this up tonight if others are busy.

[robertcoltheart]

I've raised a PR for the first issue, which is splitting the buffers here: #5623 It also optimizes the generation to only the type requested, instead of generating both open metrics and plain.

[vishweshbankwar]

closing in favor of #5623