[Prometheus] Align on the algorithm used to transform Prometheus histograms into Elasticsearch histograms #36647

tetianakravchenko · 2023-09-21T17:07:36Z

Proposed commit message

Align on the algorithm used to transform Prometheus histograms into Elasticsearch histograms Elasticsearch histograms.

The original issue for those changes was - #26903 and related discussion - elastic/apm-agent-python#1165 (comment), in this commit are addressed:

Use the preceding bucket's value for +Inf 'le',
for the first bucket only: if it has a negative 'le', use the value as-is;

Note: missing handling of negative buckets cause the error: [values] values must be in increasing order, got [-4.9E-324] but previous value was [0.0] for the kubernetes proxy metrics, as result the document that contains such a field will be dropped - related comment with the prometheus histogram sample

Additionally:

extend explanation on the counts calculation

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

[ ]

How to test this PR locally

Related issues

Relates Enhance Histogram feature implementation of Prometheus Server Integration integrations#5042
There was another issue - Adjust Prometheus histogram buckets calculation #26903 that was closed in favor of Enhance Histogram feature implementation of Prometheus Server Integration integrations#5042, that explain the motivation for this change
Relates [prometheus][remote_write] Failing to parse some histogram fields integrations#7893

Use cases

Screenshots

Logs

…lasticsearch histograms: Use the preceding bucket's value for +Inf 'le', for the first bucket only: if it has a negative 'le', use the value as-is; add test for the negative buckets; extend explanation on the counts calculation Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

botelastic · 2023-09-21T17:07:43Z

This pull request doesn't have a Team:<team> label.

mergify · 2023-09-21T17:08:35Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @tetianakravchenko? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-09-21T17:43:18Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-10-11T08:21:47.997+0000
Duration: 51 min 38 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1790
Skipped	133
Total	1923

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…s; revert unrelated changes Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

gizas · 2023-09-29T08:28:04Z

x-pack/metricbeat/module/prometheus/collector/histogram_test.go

+						SampleSum:   proto.Float64(20),
+						Bucket: []*p.Bucket{
+							{
+								UpperBound:      proto.Float64(-100),


gizas · 2023-09-29T08:43:53Z

x-pack/metricbeat/module/prometheus/remote_write/remote_write_test.go

@@ -1016,7 +1016,7 @@ func TestGenerateEventsHistogramsDifferentLabels(t *testing.T) {
 		},
 		"http_request_bytes": mapstr.M{
 			"histogram": mapstr.M{
-				"values": []float64{float64(0.125), float64(0.375), float64(0.75)},
+				"values": []float64{float64(0.125), float64(0.375), float64(0.5)},


Trying to understand this change: Why we have now 0.5?

Is it due to // Report +Inf bucket as a point, use the preceding bucket's value, and we put the last bucket?

yes, for the last bucket (+Inf 'le') is used the preceding bucket's value, instead of interpolation by adding half the difference between the previous two buckets to the second last bucket

gizas · 2023-09-29T08:44:52Z

I think this elastic/integrations#5042 can be closed when this is merged

ChrsMark

Code changes themselves look good to me.

Didn't do any validation on the algorithm but I guess it's already discussed? On this, could you link in the description's main context the link to the issue that describes this decision and the motivation?

x-pack/metricbeat/module/prometheus/collector/histogram.go

tetianakravchenko · 2023-10-10T13:32:13Z

@ChrsMark

Didn't do any validation on the algorithm but I guess it's already discussed? On this, could you link in the description's main context the link to the issue that describes this decision and the motivation?

the original issue for those changes was - #26903 and related discussion - elastic/apm-agent-python#1165 (comment)

I've added it to the description and the link to the existing parsing issues

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

tetianakravchenko · 2023-10-11T08:29:40Z

@gizas

I think this elastic/integrations#5042 can be closed when this is merged

I've left a comment that this PR covers only partially the scope of the mentioned issue - elastic/integrations#5042 (comment), do you think it can be closed anyway?

…ograms into Elasticsearch histograms (elastic#36647) * Align on the algorithm used to transform Prometheus histograms into Elasticsearch histograms: Use the preceding bucket's value for +Inf 'le', for the first bucket only: if it has a negative 'le', use the value as-is; add test for the negative buckets; extend explanation on the counts calculation Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * fix prometheus remote_write tests; regenerate istio expected documents; revert unrelated changes Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> * add link to the design/motivation of the histogram transformation logic Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co> --------- Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 21, 2023

mergify bot assigned tetianakravchenko Sep 21, 2023

tetianakravchenko added 2 commits September 22, 2023 09:22

Merge branch 'main' into prometheus-buckets-calculation

e375a40

fix prometheus remote_write tests; regenerate istio expected document…

243e6c0

…s; revert unrelated changes Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

tetianakravchenko marked this pull request as ready for review September 22, 2023 09:13

tetianakravchenko requested a review from a team as a code owner September 22, 2023 09:13

tetianakravchenko requested review from gizas and gsantoro September 22, 2023 09:13

tetianakravchenko mentioned this pull request Sep 22, 2023

Enhance Histogram feature implementation of Prometheus Server Integration elastic/integrations#5042

Open

tetianakravchenko requested review from ChrsMark and constanca-m and removed request for gsantoro September 22, 2023 14:49

tetianakravchenko mentioned this pull request Sep 26, 2023

[prometheus][remote_write] Failing to parse some histogram fields elastic/integrations#7893

Closed

gizas reviewed Sep 29, 2023

View reviewed changes

gizas approved these changes Sep 29, 2023

View reviewed changes

MichaelKatsoulis approved these changes Oct 10, 2023

View reviewed changes

ChrsMark approved these changes Oct 10, 2023

View reviewed changes

x-pack/metricbeat/module/prometheus/collector/histogram.go Show resolved Hide resolved

Merge branch 'main' into prometheus-buckets-calculation

d8fb9df

add link to the design/motivation of the histogram transformation logic

4fd2e7f

Signed-off-by: Tetiana Kravchenko <tetiana.kravchenko@elastic.co>

ChrsMark approved these changes Oct 11, 2023

View reviewed changes

tetianakravchenko added the 8.11-candidate label Oct 11, 2023

tetianakravchenko merged commit 73303ae into elastic:main Oct 11, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Prometheus] Align on the algorithm used to transform Prometheus histograms into Elasticsearch histograms #36647

[Prometheus] Align on the algorithm used to transform Prometheus histograms into Elasticsearch histograms #36647

tetianakravchenko commented Sep 21, 2023 •

edited

botelastic bot commented Sep 21, 2023

mergify bot commented Sep 21, 2023

elasticmachine commented Sep 21, 2023 •

edited by jenkins-beats-ci bot

Build stats

Test stats 🧪

gizas Sep 29, 2023

gizas Sep 29, 2023

tetianakravchenko Oct 10, 2023

gizas commented Sep 29, 2023

ChrsMark left a comment

tetianakravchenko commented Oct 10, 2023

tetianakravchenko commented Oct 11, 2023

[Prometheus] Align on the algorithm used to transform Prometheus histograms into Elasticsearch histograms #36647

[Prometheus] Align on the algorithm used to transform Prometheus histograms into Elasticsearch histograms #36647

Conversation

tetianakravchenko commented Sep 21, 2023 • edited

Proposed commit message

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

botelastic bot commented Sep 21, 2023

mergify bot commented Sep 21, 2023

elasticmachine commented Sep 21, 2023 • edited by jenkins-beats-ci bot

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

gizas Sep 29, 2023

Choose a reason for hiding this comment

gizas Sep 29, 2023

Choose a reason for hiding this comment

tetianakravchenko Oct 10, 2023

Choose a reason for hiding this comment

gizas commented Sep 29, 2023

ChrsMark left a comment

Choose a reason for hiding this comment

tetianakravchenko commented Oct 10, 2023

tetianakravchenko commented Oct 11, 2023

tetianakravchenko commented Sep 21, 2023 •

edited

elasticmachine commented Sep 21, 2023 •

edited by jenkins-beats-ci bot