lemmy icon indicating copy to clipboard operation
lemmy copied to clipboard

Make prometheus metrics a config option, not a build-time switch

Open wereii opened this issue 2 years ago • 1 comments

Requirements

  • [X] Is this a feature request? For questions or discussions use https://lemmy.ml/c/lemmy_support
  • [X] Did you check to see if this issue already exists?
  • [X] Is this only a feature request? Do not put multiple feature requests in one issue.
  • [X] Is this a backend issue? Use the lemmy-ui repo for UI / frontend issues.

Is your proposal related to a problem?

Currently one has to build the image themselves to get metrics from lemmy.

Describe the solution you'd like.

Include prometheus metrics export in standard build, moving the build time toggle into a config on/off option.

Describe alternatives you've considered.

Release official lemmy-prometheus image.

Additional context

I get that this sounds lazy but you, as the source, have all the things already in place (CI, ...) and while cloning the repo, calling docker compose up --build ... (then getting the image available to the prod server) is definitely far from difficult, it is still extra just to have something rather common, if not expectable to have by default.

wereii avatar Jul 09 '23 11:07 wereii

Seems like there's three ways to do this:

  1. The quick and easy (hacky) way: Add prometheus-metrics to the default features in the top-level Cargo.toml.
  2. Remove the prometheus-metrics feature and build it into the main build. Always collect metrics and run the server.
  3. Remove the prometheus-metrics feature and make metric collection and server running contingent upon a setting in the config file.
prometheus: {
  enabled: true
}

Option 3 is the best imo, but it's up to the maintainers what they want to do. I'll happily implement it.

As an aside, if it does get added to the main build, should the prometheus block in the config continue to be optional? I think it kind of breaks the defaults when the whole object is Optional.

andybug avatar Jul 09 '23 17:07 andybug

Any thoughts on whether to include Prometheus by default? I have some refactors I've been prepping for the Prometheus code and want to know what to do here.

@dessalines @Nutomic

andybug avatar Jul 18 '23 21:07 andybug

Im not opposed to this. I dont think an enabled field in the config is necessary, its enough to enable it if the value of Settings.prometheus is Some.

Regarding compilation it depends how much it affects build time. If there is little or no difference, the prometheus feature can be removed entirely. But if it slows down build siginficantly, then it would be better to leave the feature as is and pass --feature prometheus-metrics in the ci release build. So it requires some measurements before deciding.

Nutomic avatar Jul 20 '23 09:07 Nutomic

For the impact on build times:

> hyperfine --warmup 1 --runs 1 --prepare "cargo clean" --command-name default-build "cargo build --features default" --command-name prometheus-build "cargo build --features prometheus-metrics"
Benchmark 1: default-build
  Time (abs ≡):        107.239 s               [User: 454.757 s, System: 35.856 s]

Benchmark 2: prometheus-build
  Time (abs ≡):        108.878 s               [User: 471.422 s, System: 40.106 s]

Summary
  default-build ran
    1.02 times faster than prometheus-build

andybug avatar Jul 22 '23 15:07 andybug

I'm going to include the change to make the Prometheus metrics part of the default build in an upcoming PR. The work in progress branch is here.

andybug avatar Jul 26 '23 02:07 andybug

I'm going to include the change to make the Prometheus metrics part of the default build in an upcoming PR. The work in progress branch is here.

This would be really great :)

Morethanevil avatar Jul 26 '23 18:07 Morethanevil