[Prometheus] Add unit tests for parsing metric families #36669

constanca-m · 2023-09-25T11:06:18Z

What does this PR do?

The previous naming we were using for metrics, especially for open metrics, had a few problems. Since there were no unit tests checking the way we were parsing the metrics, we did not see this before. So this PR does 2 things:

It solidifies the way we name our metrics family (see next section)
Adds the unit tests to make sure everything works as expected

The specification for OpenMetrics can be found here.

Naming: metric family name and metrics names

For OpenMetric Parser (see suffixes here):

counter:
- If the name of the metric sample does not include _total or _created, then the metric is ignored. These are the only possible options.
- If the name of the metric sample includes a suffix, we do one of the two:
  - The metric FAMILY name is the name without the suffix IF it exists in the families dictionary already (we know this way we received metadata for that)
  - The metric family name is the same as the metric name with the suffix otherwise.
gauge: there are no rules here, no suffixes are necessary
histogram: we check for the acceptable suffixes
gaugehistogram: same as histogram
summary: same logic as for histogram
info: we apply the same reasoning as we did for counter
stateset: no rules for the suffixes
unknown: no rules for the suffixes

For Prometheus Parser:

counter: we do not remove the suffix, unlike with OpenMetrics
gauge
histogram: same logic as the above
summary: same logic as the above

Please check the textparse_test file to see clear examples of this.

Related issues

Relates to #36537.

mergify · 2023-09-25T11:06:56Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @constanca-m? 🙏.
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

constanca-m · 2023-09-25T11:06:59Z

metricbeat/helper/prometheus/textparse.go

@@ -351,7 +352,7 @@ func isBucket(name string) bool {
 	return strings.HasSuffix(name, suffixBucket)
 }

-func summaryMetricName(name string, s float64, qv string, lbls string, t *int64, summariesByName map[string]map[string]*OpenMetric) (string, *OpenMetric) {
+func summaryMetricName(name string, s float64, qv string, lbls string, summariesByName map[string]map[string]*OpenMetric) (string, *OpenMetric) {


Parameter t wasn't being used, that is why it is removed.

constanca-m · 2023-09-25T11:09:12Z

metricbeat/helper/prometheus/textparse.go

@@ -653,8 +658,6 @@ func ParseMetricFamilies(b []byte, contentType string, ts time.Time) ([]*MetricF
 			metricFamiliesByName[lookupMetricName] = fam
 		}

-		fam.Name = &metricName


This line needed to be removed, because it was causing errors.

Example: if we had this metric:

# HELP cc_seconds A counter # TYPE cc_seconds counter # UNIT cc_seconds seconds cc_seconds_total 1.0 cc_seconds_created 123.456

The metric would be renamed to cc_seconds_total and after to cc_seconds_created. When in reality, the metric name is cc_seconds (without the suffix). And then the metric.Metric is a list of metrics, where both are included.
This test case is in the test file.

elasticmachine · 2023-09-25T11:57:13Z

💚 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-03T08:14:51.488+0000
Duration: 51 min 35 sec

Test stats 🧪

Test	Results
Failed	0
Passed	4411
Skipped	902
Total	5313

💚 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!)

belimawr

I'm far from being an expert in this bit of the codebase, but LGTM

constanca-m · 2023-09-28T09:26:38Z

I will close this PR for now while we resolve the naming conflicts.

mergify · 2023-09-29T10:07:32Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @constanca-m? 🙏.
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

gizas · 2023-09-29T13:01:50Z

metricbeat/helper/prometheus/textparse.go

+				// Remove the two possible suffixes, _created and _total
+				if isTotal(metricName) {
+					lookupMetricName = strings.TrimSuffix(metricName, suffixTotal)
+				} else {


Suggested change

} else {

} isCreated(metricName) {

Because of this condition above if contentType == OpenMetricsType && !isTotal(metricName) && !isCreated(metricName) we already know that if we are in the else, the suffix is _created @gizas

constanca-m · 2023-09-29T13:06:12Z

metricbeat/helper/openmetrics/openmetrics_test.go

@@ -97,8 +97,8 @@ metrics_one_count_total{name="john",surname="williams"} 2
 metrics_one_count_total{name="jahn",surname="baldwin",age="30"} 3
 `

-	openMetricsCounterKeyLabelWithNaNInf = `# TYPE metrics_one_count_errors counter
-metrics_one_count_errors{name="jane",surname="foster"} 1
+	openMetricsCounterKeyLabelWithNaNInf = `# TYPE metrics_one_count_errors_total counter


According to this, counter types MUST have the suffix _total. I had to fix this to pass the test.

constanca-m · 2023-09-29T13:45:41Z

The changes on OpenMetrics sample files are the result from running go -test data. It was causing the tests to fail, I don't know why it was only picked up on this PR.

gizas · 2023-10-03T08:00:05Z

metricbeat/helper/prometheus/textparse_test.go

+# TYPE metric_info info
+metric_info 2
+# TYPE metric_without_suffix info
+metric_without_suffix 3


This is dropped right?

gizas · 2023-10-03T08:03:34Z

metricbeat/helper/prometheus/textparse_test.go

+	require.ElementsMatch(t, expected, result)
+}
+
+func TestHistogramPrometheus(t *testing.T) {


Dummy question: Why we need both this test and TestHistogramOpenMetrics?
When this will be called?

For open metrics, we use a parser that is different than the one we use for prometheus metrics. That is decided by the content we give to the parse metrics function. So we are just checking the expected results for the two possible parsers: the PromParser and the OpenMetricsParser.

gizas

Constanca make sure that the link https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md

is added in the description of this PR, to have it handy for future reference

ChrsMark

Changes lgtm.

However since this changes significantly the core functionality I would feel more confident if we had a manual testing phase in place as a double check.

* Add tests for parsing metrics. * Remove _info suffix. * Add unit tests * Add unit tests * Add unit tests * Add license header * Fix goimports. * Fix openmetrics tests.

Add tests for parsing metrics.

9f57d4a

constanca-m added the :Testing label Sep 25, 2023

constanca-m requested a review from a team September 25, 2023 11:06

constanca-m requested a review from a team as a code owner September 25, 2023 11:06

constanca-m self-assigned this Sep 25, 2023

constanca-m requested review from belimawr and faec September 25, 2023 11:06

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

constanca-m commented Sep 25, 2023

View reviewed changes

constanca-m mentioned this pull request Sep 25, 2023

[Prometheus] Update prometheus histogram #36537

Merged

6 tasks

constanca-m added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Sep 25, 2023

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

constanca-m requested a review from ChrsMark September 25, 2023 11:10

constanca-m and others added 2 commits September 25, 2023 14:02

Merge branch 'main' into add-prometheus-tests

b7b76eb

Remove _info suffix.

4a2c943

belimawr approved these changes Sep 25, 2023

View reviewed changes

constanca-m closed this Sep 28, 2023

constanca-m added 2 commits September 29, 2023 12:03

Add unit tests

b5153df

Add unit tests

1370449

constanca-m reopened this Sep 29, 2023

constanca-m added 3 commits September 29, 2023 12:07

Add unit tests

a0b459c

Add license header

c140872

Fix goimports.

82050d8

gizas reviewed Sep 29, 2023

View reviewed changes

Fix openmetrics tests.

3d32656

constanca-m requested a review from a team as a code owner September 29, 2023 13:05

constanca-m commented Sep 29, 2023

View reviewed changes

gizas reviewed Oct 3, 2023

View reviewed changes

gizas approved these changes Oct 3, 2023

View reviewed changes

Merge branch 'main' into add-prometheus-tests

76cf65e

ChrsMark approved these changes Oct 3, 2023

View reviewed changes

constanca-m merged commit 25624ab into elastic:main Oct 3, 2023
7 checks passed

constanca-m deleted the add-prometheus-tests branch October 3, 2023 09:30

constanca-m mentioned this pull request May 24, 2024

Prometheus parser incorrectly classifies counters as histograms #39705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Prometheus] Add unit tests for parsing metric families #36669

[Prometheus] Add unit tests for parsing metric families #36669

constanca-m commented Sep 25, 2023 •

edited

Loading

mergify bot commented Sep 25, 2023

constanca-m Sep 25, 2023

constanca-m Sep 25, 2023

elasticmachine commented Sep 25, 2023 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

belimawr left a comment

constanca-m commented Sep 28, 2023

mergify bot commented Sep 29, 2023

gizas Sep 29, 2023

constanca-m Sep 29, 2023

constanca-m Sep 29, 2023

constanca-m commented Sep 29, 2023

gizas Oct 3, 2023

constanca-m Oct 3, 2023

gizas Oct 3, 2023

constanca-m Oct 3, 2023

gizas left a comment

ChrsMark left a comment

[Prometheus] Add unit tests for parsing metric families #36669

[Prometheus] Add unit tests for parsing metric families #36669

Conversation

constanca-m commented Sep 25, 2023 • edited Loading

What does this PR do?

Naming: metric family name and metrics names

Related issues

mergify bot commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

belimawr left a comment

Choose a reason for hiding this comment

constanca-m commented Sep 28, 2023

mergify bot commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

constanca-m commented Sep 29, 2023

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

gizas left a comment

Choose a reason for hiding this comment

ChrsMark left a comment

Choose a reason for hiding this comment

constanca-m commented Sep 25, 2023 •

edited

Loading

elasticmachine commented Sep 25, 2023 •

edited by jenkins-beats-ci bot

Loading