Filter expected metrics as well in CollectAndCompare

[DariaKunoichi]

Problem:
Implementation for GatherAndCompare does not match documentation. Documentation says that if you provide any metricNames it filters both provided and expected metrics. But only provided metrics are filtered in compareMetricFamilies function.

Here is my usecase: https://github.com/fluxninja/aperture/blob/main/pkg/otelcollector/metricsprocessor/processor_test.go#L154

I manually add <split> in the expectedMetrics string so I don't have to provide multiple expectedMetrics variables.

[DariaKunoichi]

@bwplotka Please take a look at this quick fix.

[kakkoyun]

@DariaKunoichi It would be nice to have some context in the PR description. What's the expected behavior and what is not working?

[bwplotka]

Awesome, thanks for contributing! ❤️

A description of the PR would be nice, as now we have to guess what you are trying to fix here. 🙈

I guess that you would like the provided expected slice to be affected by filtering. Thanks to that, functions like ScrapeAndCompare could accept any bigger, arbitrary expected stream of bytes. However, given expected stream is usually provided as a synthetic sting (typically hand-crafted), you can filter it on your own - that's why it was not filtered before.

So to push this change, ideally:

• Please tell us what use case you have, it will help motivate this change.
• Can we modify the TestScrapeAndCompare unit test to test this change?

[DariaKunoichi]

Thanks for the pointers, guys. I need to remember what was the issue and I'll add the description and modify the PR if it's needed.
@bwplotka Added testcase and the description.

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[bwplotka]

Thx and sorry for lag, will check today (:

[kakkoyun]

LGTM 🚀

[bwplotka]

Thanks! One important improved to test is needed if I am not wrong 🤗

[bwplotka]

Can we expose different some_total{} so we can actually test filtering? Otherwise without your change this test would pass as well if I am understanding this correctly.

[DariaKunoichi]

Ok, sure, will change.

[SD145]

Reply Edit

…

On Tue, Mar 21, 2023, 19:20 Daria Bialobrzeska ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In prometheus/testutil/testutil_test.go <#1143 (comment)> : > + const expected = ` + # HELP some_total A value that represents a counter. + # TYPE some_total counter + + some_total{ label1 = "value1" } 1 + + # HELP some_total2 A value that represents a counter. + # TYPE some_total2 counter + + some_total2{ label1 = "value1" } 1 + ` + + expectedReader := strings.NewReader(expected) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintln(w, expected) Ok, sure, will change. — Reply to this email directly, view it on GitHub <#1143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZFPUNAPBOTX72HX4X3OAY3W5GWYRANCNFSM6AAAAAAQ3UBFGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[bwplotka]

Thanks!

Great job!

[bwplotka]

We have one flaky test #765, merging