Feature: Add option to exclude consumer from Stream metrics #906

nantiferov · 2024-05-18T06:43:12Z

Tested it manually, looks like working properly, PTAL.

Probably need to add more tests, can check it later.

Amount of metrics on test redis:

without streams metrics collect - 260
with REDIS_EXPORTER_CHECK_STREAMS=* - 680
with this feature enabled in addition to previous - 430

Fixes: #902

oliver006

Thanks for the PR!
Looks good, minor suggestions re: naming to make it more clear what you're excluding (metrics) but I think you're going in the right direction.

oliver006 · 2024-05-19T03:12:28Z

README.md

@@ -148,6 +148,7 @@ Prometheus uses file watches and all changes to the json file are applied immedi
 | check-single-keys       | REDIS_EXPORTER_CHECK_SINGLE_KEYS       | Comma separated list of keys to export value and length/size, eg: `db3=user_count` will export key `user_count` from db `3`. db defaults to `0` if omitted.  The keys specified with this flag will be looked up directly without any glob pattern matching.  Use this option if you don't need glob pattern matching;  it is faster than `check-keys`.                                                                                                                                                                                           |
 | check-streams           | REDIS_EXPORTER_CHECK_STREAMS           | Comma separated list of stream-patterns to export info about streams, groups and consumers. Syntax is the same as `check-keys`.                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | check-single-streams    | REDIS_EXPORTER_CHECK_SINGLE_STREAMS    | Comma separated list of streams to export info about streams, groups and consumers. The streams specified with this flag will be looked up directly without any glob pattern matching.  Use this option if you don't need glob pattern matching;  it is faster than `check-streams`.                                                                                                                                                                                                                                                              |
+| streams-exclude-consumer    | REDIS_EXPORTER_STREAMS_EXCLUDE_CONSUMER    | Don't collect per consumer metrics for streams (decreases amount of metrics and cardinality).                                                                                                                                                                                                                                                                                                                                                                                                                   |


REDIS_EXPORTER_STREAMS_EXCLUDE_CONSUMER -> REDIS_EXPORTER_STREAMS_EXCLUDE_CONSUMER_METRICS ?

Yep, sure.

My only concern was that it would be quite long, but not too much I think)

Done, committed changes.

Apparently tests are failing, but I don't see exact reason.

In the end of output only:

FAIL coverage: 90.0% of statements FAIL github.com/oliver006/redis_exporter/exporter 22.273s FAIL

And before something looks like errors, but nothing says FAIL

=== RUN TestParseKeyArg/invalid_args_with_args_separator_skipped time="2024-05-20T17:54:09Z" level=error msg="parseKeyArg(): Empty value parsed in pair '=', skip" === RUN TestParseKeyArg/empty_args_with_comma_separators_skipped === RUN TestParseKeyArg/multiple_invalid_args_skipped time="2024-05-20T17:54:09Z" level=error msg="parseKeyArg(): Empty value parsed in pair '=', skip" time="2024-05-20T17:54:09Z" level=error msg="parseKeyArg(): Empty value parsed in pair '=', skip" === RUN TestParseKeyArg/empty_key_with_args_separator_skipped time="2024-05-20T17:54:09Z" level=error msg="parseKeyArg(): Empty value parsed in pair '0=', skip" === RUN TestParseKeyArg/empty_database_with_args_separator_skipped time="2024-05-20T17:54:09Z" level=error msg="parseKeyArg(): Empty value parsed in pair '=my-key', skip" === RUN TestParseKeyArg/string_database_index keys_test.go:265: Expected failure for test: string_database_index, got err: Invalid database index for db "wrong": strconv.Atoi: parsing "wrong": invalid syntax === RUN TestParseKeyArg/prefixed_string_database_index keys_test.go:265: Expected failure for test: prefixed_string_database_index, got err: Invalid database index for db "wrong": strconv.Atoi: parsing "wrong": invalid syntax === RUN TestParseKeyArg/wrong_args_count keys_test.go:265: Expected failure for test: wrong_args_count, got err: invalid key list argument: wrong=wrong=wrong === RUN TestParseKeyArg/wrong_args keys_test.go:265: Expected failure for test: wrong_args, got err: invalid key list argument: wrong=wrong=1 === RUN TestParseKeyArg/negative_database_index keys_test.go:265: Expected failure for test: negative_database_index, got err: Invalid database index for db "-1": %!s(<nil>)

oliver006 · 2024-05-19T03:12:41Z

exporter/exporter.go

@@ -55,6 +55,7 @@ type Options struct {
 	CheckSingleKeys           string
 	CheckStreams              string
 	CheckSingleStreams        string
+	StreamsExcludeConsumer    bool


Same, add Metrics ?

oliver006 · 2024-05-20T23:07:49Z

Try rebasing your PR, I pushed a fix to master last week to fix an issue with an outdated Redis docker container.

coveralls · 2024-05-21T06:26:52Z

Pull Request Test Coverage Report for Build 352

Details

8 of 83 (9.64%) changed or added relevant lines in 3 files are covered.
13 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.06%) to 84.172%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
main.go	0	75	0.0%

Files with Coverage Reduction	New Missed Lines	%
exporter/exporter.go	13	94.44%

Totals
Change from base Build 344:	-0.06%
Covered Lines:	1973
Relevant Lines:	2344

💛 - Coveralls

nantiferov · 2024-05-21T06:30:56Z

Try rebasing your PR, I pushed a fix to master last week to fix an issue with an outdated Redis docker container.

That helped, thank you.

Though coverage decreased, I will check later today to add tests.

nantiferov · 2024-05-21T14:53:01Z

Added tests for new option, but coverage still decreases.

oliver006

PR looks good, minor comment re: the README but otherwise looks good

oliver006 · 2024-05-24T18:43:08Z

README.md

@@ -148,6 +148,7 @@ Prometheus uses file watches and all changes to the json file are applied immedi
 | check-single-keys       | REDIS_EXPORTER_CHECK_SINGLE_KEYS       | Comma separated list of keys to export value and length/size, eg: `db3=user_count` will export key `user_count` from db `3`. db defaults to `0` if omitted.  The keys specified with this flag will be looked up directly without any glob pattern matching.  Use this option if you don't need glob pattern matching;  it is faster than `check-keys`.                                                                                                                                                                                           |
 | check-streams           | REDIS_EXPORTER_CHECK_STREAMS           | Comma separated list of stream-patterns to export info about streams, groups and consumers. Syntax is the same as `check-keys`.                                                                                                                                                                                                                                                                                                                                                                                                                   |
 | check-single-streams    | REDIS_EXPORTER_CHECK_SINGLE_STREAMS    | Comma separated list of streams to export info about streams, groups and consumers. The streams specified with this flag will be looked up directly without any glob pattern matching.  Use this option if you don't need glob pattern matching;  it is faster than `check-streams`.                                                                                                                                                                                                                                                              |
+| streams-exclude-consumer    | REDIS_EXPORTER_STREAMS_EXCLUDE_CONSUMER_METRICS    | Don't collect per consumer metrics for streams (decreases amount of metrics and cardinality).                                                                                                                                                                                                                                                                                                                                                                                                                   |


streams-exclude-consumer --> streams-exclude-consumer-metrics

Thanks for noticing. Missed update here.

Now should be good.

oliver006 · 2024-05-24T18:44:54Z

main.go

+		checkKeyGroups                = flag.String("check-key-groups", getEnv("REDIS_EXPORTER_CHECK_KEY_GROUPS", ""), "Comma separated list of lua regex for grouping keys")
+		checkStreams                  = flag.String("check-streams", getEnv("REDIS_EXPORTER_CHECK_STREAMS", ""), "Comma separated list of stream-patterns to export info about streams, groups and consumers, searched for with SCAN")
+		checkSingleStreams            = flag.String("check-single-streams", getEnv("REDIS_EXPORTER_CHECK_SINGLE_STREAMS", ""), "Comma separated list of single streams to export info about streams, groups and consumers")
+		streamsExcludeConsumerMetrics = flag.Bool("streams-exclude-consumer-metrics", getEnvBool("REDIS_EXPORTER_STREAMS_EXCLUDE_CONSUMER_METRICS", false), "Don't collect per consumer metrics for streams (decreases cardinality)")


streams-exclude-consumer-metrics - here you got it right :-)

exporter/streams_test.go

…udeConsumer test

oliver006

Looks good now, thanks for the PR!

nantiferov · 2024-05-30T09:39:58Z

@oliver006 when you have time, could you please merge this PR if all looks good now and release a new version?

oliver006 · 2024-05-30T15:38:37Z

Thanks for the nudge.
Do you think we can get #911 sorted and into the next release as well?

nantiferov · 2024-05-30T15:47:57Z

Yep, probably can check it later today.

…6#906) * Feature: Add option to exclude consumer from Stream metrics

oliver006 · 2024-05-31T06:09:57Z

Released v1.60.0 which includes this and your other PRs - nice work, thanks for all your contributions!

nantiferov · 2024-05-31T08:10:10Z

Thank you!

Feature: Add option to exclude consumer from Stream metrics

a78079c

Fixes: oliver006#902

oliver006 reviewed May 19, 2024

View reviewed changes

Clarify exclude consumer metrics from streams

1b2825c

Merge branch 'oliver006:master' into feature-exclude-consumer-stream

d97b9bb

Add TestStreamsExtractStreamMetricsExcludeConsumer to cover new option

60ca0d6

oliver006 reviewed May 24, 2024

View reviewed changes

Update Readme for new streams-exclude-consumer-metrics option

7ee9fed

oliver006 reviewed May 26, 2024

View reviewed changes

exporter/streams_test.go Show resolved Hide resolved

Add check for excluded metrics to TestStreamsExtractStreamMetricsExcl…

cec04c8

…udeConsumer test

oliver006 approved these changes May 28, 2024

View reviewed changes

oliver006 mentioned this pull request May 28, 2024

Feature: Add option to skip collecting latency histogram metrics #911

Closed

oliver006 merged commit 2c968d5 into oliver006:master May 30, 2024
4 of 5 checks passed

Nklya pushed a commit to Nklya/redis_exporter that referenced this pull request May 30, 2024

Feature: Add option to exclude consumer from Stream metrics (oliver00…

3b3906c

…6#906) * Feature: Add option to exclude consumer from Stream metrics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Add option to exclude consumer from Stream metrics #906

Feature: Add option to exclude consumer from Stream metrics #906

nantiferov commented May 18, 2024

oliver006 left a comment •

edited

oliver006 May 19, 2024

nantiferov May 20, 2024

nantiferov May 20, 2024

nantiferov May 20, 2024 •

edited

oliver006 May 19, 2024

nantiferov May 20, 2024

oliver006 commented May 20, 2024

coveralls commented May 21, 2024 •

edited

nantiferov commented May 21, 2024

nantiferov commented May 21, 2024

oliver006 left a comment

oliver006 May 24, 2024

nantiferov May 25, 2024

oliver006 May 24, 2024

oliver006 left a comment

nantiferov commented May 30, 2024

oliver006 commented May 30, 2024

nantiferov commented May 30, 2024

oliver006 commented May 31, 2024 •

edited

nantiferov commented May 31, 2024

Feature: Add option to exclude consumer from Stream metrics #906

Feature: Add option to exclude consumer from Stream metrics #906

Conversation

nantiferov commented May 18, 2024

oliver006 left a comment • edited

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

nantiferov May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver006 commented May 20, 2024

coveralls commented May 21, 2024 • edited

Pull Request Test Coverage Report for Build 352

Details

💛 - Coveralls

nantiferov commented May 21, 2024

nantiferov commented May 21, 2024

oliver006 left a comment

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

oliver006 left a comment

Choose a reason for hiding this comment

nantiferov commented May 30, 2024

oliver006 commented May 30, 2024

nantiferov commented May 30, 2024

oliver006 commented May 31, 2024 • edited

nantiferov commented May 31, 2024

oliver006 left a comment •

edited

nantiferov May 20, 2024 •

edited

coveralls commented May 21, 2024 •

edited

oliver006 commented May 31, 2024 •

edited