Skip to content
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

Consolidate metrics code in a single package, add metrics prefix config option #55

Merged

Conversation

Evedel
Copy link

@Evedel Evedel commented Jan 31, 2023

Currently, the Prometheus metrics code is slightly all over the place, which is making it difficult to figure out the code and available metrics.

This PR:

  1. proposes to extract all the metrics code into a separate metrics package pkg/metrics/metrics.go
    • no more prom imports and variables all over different packages
    • the metrics package is then dependency injected via input parameter, enabling testing of metrics
  2. metricsNamePrefix is added as an optional parameter
    • if empty => all the old metric names are used, so fully backward compatible, but will warn user to set a prefix manually
      k logs event-exporter-678958f5dc-c2wwd
      {"level":"warn","caller":"/app/pkg/exporter/config.go:86","message":"metrics name prefix is empty, setting config.metricsNamePrefix='event_exporter_' is recommended"}
      
    • if set => all the metrics will be prefixed, easy to discover available
      image
    • config validation is tested

@Evedel
Copy link
Author

Evedel commented Jan 31, 2023

While this pr is backward compatible, it seems to me that ideally, metrics are planned for renaming in some future major version bump.

  1. events_sent => events_received (it is more accurate description of what this metric is counting and will be consistent with the logging too)
  2. all metrics are prefixed by default (as it is a prom standard)
  3. all metrics are also suffixed with _total/_count (same prom standard, but for a unit-less accumulating count)

@mustafaakin
Copy link

Hello, thanks for the PR, nice change. Metrics definitely look much better now.

@mustafaakin mustafaakin merged commit 9aa12cf into resmoio:master Mar 17, 2023
@Evedel Evedel deleted the consolidate-metrics-in-single-package branch March 19, 2023 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants