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

[prometheus] Fix issue with corrupted buffers when reading both OpenMetrics and plain text formats #5623

Merged

Conversation

robertcoltheart
Copy link
Contributor

@robertcoltheart robertcoltheart commented May 16, 2024

Fixes #5515

Changes

Separate prometheus buffers into plain text and openmetrics. This is the first part of splitting #5517. It also optimizes the generation of the buffers, and only generates the requested buffer, rather than generating both simultaneously.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes

@robertcoltheart robertcoltheart requested a review from a team May 16, 2024 11:07
Copy link

codecov bot commented May 16, 2024

Codecov Report

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

Project coverage is 85.71%. Comparing base (6250307) to head (2e62fde).
Report is 229 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5623      +/-   ##
==========================================
+ Coverage   83.38%   85.71%   +2.33%     
==========================================
  Files         297      254      -43     
  Lines       12531    11036    -1495     
==========================================
- Hits        10449     9460     -989     
+ Misses       2082     1576     -506     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 85.70% <76.59%> (?)
unittests-Solution-Stable 85.69% <76.59%> (?)
unittests-Unstable-Core 19.95% <76.59%> (?)

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

Files Coverage Δ
...metheus.AspNetCore/PrometheusExporterMiddleware.cs 70.73% <100.00%> (+0.73%) ⬆️
....Prometheus.HttpListener/PrometheusHttpListener.cs 83.13% <100.00%> (+1.42%) ⬆️
...tpListener/Internal/PrometheusCollectionManager.cs 77.93% <73.17%> (+0.15%) ⬆️

... and 113 files with indirect coverage changes

@reyang
Copy link
Member

reyang commented May 16, 2024

Thanks @robertcoltheart!

@robertcoltheart
Copy link
Contributor Author

Done, please review

Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks @robertcoltheart!

@CodeBlanch CodeBlanch added pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Prometheus.HttpListener Issues related to OpenTelemetry.Exporter.Prometheus.HttpListener NuGet package labels May 17, 2024
@CodeBlanch CodeBlanch changed the title Fix issue with corrupted buffers when reading OpenMetrics and plain text from prometheus [prometheus] Fix issue with corrupted buffers when reading both OpenMetrics and plain text formats May 17, 2024
@CodeBlanch CodeBlanch merged commit 8177a39 into open-telemetry:main May 17, 2024
37 checks passed
miniduikboot added a commit to miniduikboot/Boot.Metrics that referenced this pull request May 23, 2024
In production I ran into some issues where Prometheus refused reading
the buffer with `expected label value, got "INVALID"`. Update to latest
alpha to fix this issue

See open-telemetry/opentelemetry-dotnet#5623
@bbrandt
Copy link

bbrandt commented Jun 21, 2024

(orig)
This change results in a regression causing no output to be displayed when IncreaseBufferSize(...) is needed. The issue seems to be that IncreaseBufferSize(...) is taking a reference to the buffer local variable, but not to this.openMetricsBuffer or this.plainTextBuffer, so these members do not get the updated content after a resize occurs. Then on 294 when we try to access this.openMetricsBuffer or this.plainTextBuffer it is the original buffer instead of the resized buffer.

Looks like this is fixed by #5676, which has not yet been merged.

@Kielek @vishweshbankwar @CodeBlanch What is needed to get #5676 merged?

@robertcoltheart robertcoltheart deleted the feature/support-both-formats branch June 21, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Prometheus.HttpListener Issues related to OpenTelemetry.Exporter.Prometheus.HttpListener NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus exporter sometimes produces malformed data
6 participants