-
Notifications
You must be signed in to change notification settings - Fork 915
Feature: add include-go-runtime-metrics flag to enable Go runtime metrics #1035
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
base: master
Are you sure you want to change the base?
Conversation
32edc40
to
32b83c3
Compare
32b83c3
to
0b6d145
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1035 +/- ##
==========================================
- Coverage 82.11% 81.96% -0.15%
==========================================
Files 19 19
Lines 3013 3017 +4
==========================================
- Hits 2474 2473 -1
- Misses 427 431 +4
- Partials 112 113 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pull Request Test Coverage Report for Build 17541633306Details
💛 - Coveralls |
0b6d145
to
2e15fe2
Compare
README.md
Outdated
@@ -186,7 +186,8 @@ scrape_configs: | |||
| connection-timeout | REDIS_EXPORTER_CONNECTION_TIMEOUT | Timeout for connection to Redis instance, defaults to "15s" (in Golang duration format) | | |||
| web.listen-address | REDIS_EXPORTER_WEB_LISTEN_ADDRESS | Address to listen on for web interface and telemetry, defaults to `0.0.0.0:9121`. | | |||
| web.telemetry-path | REDIS_EXPORTER_WEB_TELEMETRY_PATH | Path under which to expose metrics, defaults to `/metrics`. | | |||
| redis-only-metrics | REDIS_EXPORTER_REDIS_ONLY_METRICS | Whether to also export go runtime metrics, defaults to false. | | |||
| redis-only-metrics | REDIS_EXPORTER_REDIS_ONLY_METRICS | Whether to export only Redis metrics (omit Go runtime metrics), defaults to false. | | |||
| extended-go-metrics | REDIS_EXPORTER_EXTENDED_GO_METRICS | Whether to export all Go runtime metrics, defaults to false. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming is hard 😅
Wdyt if this option renamed to enable-go-runtime-metrics
or include-go-runtime-metrics
or go-runtime-metrics
?
I don't think they're extended. We're basically either collecting redis_exporter process metrics, which are technically Go runtime metrics, but they could be called just process metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! i like that.
you're right, we got Golang runtime metrics, and then we got "process metrics"
I like include-go-runtime-metrics
- what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think it sounds good.
Changed PR with renamed variables, PTAL when have time.
Thanks for this PR - I'm super swamped this week but will try to review it as soon as possible. |
Thank you. No worries, it's not urgent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your thinking about runtime metrics (instead of "extended") vs process metrics, that makes sense, let's do that
code change itself looks good
main.go
Outdated
@@ -124,15 +124,19 @@ func setupLogging(isDebug bool, logLevel, logFormat string) error { | |||
} | |||
|
|||
// createPrometheusRegistry creates and configures a Prometheus registry | |||
func createPrometheusRegistry(redisMetricsOnly bool) *prometheus.Registry { | |||
func createPrometheusRegistry(redisMetricsOnly, extendedGoMetrics bool) *prometheus.Registry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotta rename this if we go with "runtime metrics"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, done
2e15fe2
to
68f9414
Compare
@@ -209,9 +210,9 @@ scrape_configs: | |||
| check-key-groups | REDIS_EXPORTER_CHECK_KEY_GROUPS | Comma separated list of [LUA regexes](https://www.lua.org/pil/20.1.html) for classifying keys into groups. The regexes are applied in specified order to individual keys, and the group name is generated by concatenating all capture groups of the first regex that matches a key. A key will be tracked under the `unclassified` group if none of the specified regexes matches it. | | |||
| max-distinct-key-groups | REDIS_EXPORTER_MAX_DISTINCT_KEY_GROUPS | Maximum number of distinct key groups that can be tracked independently *per Redis database*. If exceeded, only key groups with the highest memory consumption within the limit will be tracked separately, all remaining key groups will be tracked under a single `overflow` key group. | | |||
| config-command | REDIS_EXPORTER_CONFIG_COMMAND | What to use for the CONFIG command, defaults to `CONFIG`, , set to "-" to skip config metrics extraction. | | |||
| basic-auth-username | REDIS_EXPORTER_BASIC_AUTH_USERNAME | Username for Basic Authentication with the redis exporter needs to be set together with basic-auth-password to be effective |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is related to trailing spaces I think. My editor automatically removed them.
Can rollback this one if needed.
@@ -378,4 +379,3 @@ Or you can bring up the stack, run the tests, and then tear down the stack, all | |||
## Communal effort | |||
|
|||
Open an issue or PR if you have more suggestions, questions or ideas about what to add. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Editor also removed extra newline here. Can rollback as well.
68f9414
to
14e32f5
Compare
README.md
Outdated
@@ -186,7 +186,8 @@ scrape_configs: | |||
| connection-timeout | REDIS_EXPORTER_CONNECTION_TIMEOUT | Timeout for connection to Redis instance, defaults to "15s" (in Golang duration format) | | |||
| web.listen-address | REDIS_EXPORTER_WEB_LISTEN_ADDRESS | Address to listen on for web interface and telemetry, defaults to `0.0.0.0:9121`. | | |||
| web.telemetry-path | REDIS_EXPORTER_WEB_TELEMETRY_PATH | Path under which to expose metrics, defaults to `/metrics`. | | |||
| redis-only-metrics | REDIS_EXPORTER_REDIS_ONLY_METRICS | Whether to also export go runtime metrics, defaults to false. | | |||
| redis-only-metrics | REDIS_EXPORTER_REDIS_ONLY_METRICS | Whether to export only Redis metrics (omit Go process+runtime metrics), defaults to false. | | |||
| include-go-runtime-metrics | REDIS_INCLUDE_GO_RUNTIME_METRICS | Whether to include Go runtime metrics, defaults to false. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REDIS_INCLUDE_GO_RUNTIME_METRICS -> REDIS_EXPORTER_INCLUDE_GO_RUNTIME_METRICS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thank you 👍
main.go
Outdated
@@ -174,7 +178,8 @@ func main() { | |||
exportClientList = flag.Bool("export-client-list", getEnvBool("REDIS_EXPORTER_EXPORT_CLIENT_LIST", false), "Whether to scrape Client List specific metrics") | |||
exportClientPort = flag.Bool("export-client-port", getEnvBool("REDIS_EXPORTER_EXPORT_CLIENT_PORT", false), "Whether to include the client's port when exporting the client list. Warning: including the port increases the number of metrics generated and will make your Prometheus server take up more memory") | |||
showVersion = flag.Bool("version", false, "Show version information and exit") | |||
redisMetricsOnly = flag.Bool("redis-only-metrics", getEnvBool("REDIS_EXPORTER_REDIS_ONLY_METRICS", false), "Whether to also export go runtime metrics") | |||
redisMetricsOnly = flag.Bool("redis-only-metrics", getEnvBool("REDIS_EXPORTER_REDIS_ONLY_METRICS", false), "Whether to export only Redis metrics (omit Go process+runtime metrics)") | |||
inclGoRuntimeMetrics = flag.Bool("include-go-runtime-metrics", getEnvBool("REDIS_INCLUDE_GO_RUNTIME_METRICS", false), "Whether to include Go runtime metrics") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
REDIS_EXPORTER_INCLUDE_GO_RUNTIME_METRICS
PR looks good, just change REDIS_INCLUDE_GO_RUNTIME_METRICS to REDIS_EXPORTER_INCLUDE_GO_RUNTIME_METRICS |
14e32f5
to
adb19af
Compare
…rics * Add include-go-runtime-metrics flag to enable Go runtime metrics, default disabled * Add test suite for inclGoRuntimeMetrics to main_test.go * Fix description for redisMetricsOnly flag Fix oliver006#1034
adb19af
to
02b2548
Compare
Fix #1034