naiserator Enable Prometheus scraping by default

The annotation prometheus.io/scrape is set to false by default. Should we just enable this for everyone by default? /cc @jhrv @terjesannum

Sep 23 '19 07:09 kimtore

Jeg stemmer nei, da det er kjekt å kunne teste ting uten at man må sette den til false for å unngå unødvendig scraping.

Sep 23 '19 07:09 Kyrremann

Default-verdi bør reflektere hva vi mener default oppførsel bør være. At det er litt kjekt når man tester ting mener jeg ikke veier opp for at dette må settes eksplisitt for nesten alle apper.

Sep 23 '19 08:09 jhrv

Jeg synes ikke det bør være default på

Sep 23 '19 08:09 terjesannum

Jeg synes ikke det bør være default på

Hvorfor ikke?

Sep 23 '19 09:09 jhrv

Alle apper har ikke noe prometheus endepunkt, da synes jeg det er ryddigere at de som har det skrur på scraping

Sep 23 '19 10:09 terjesannum

Trodde ca alle apper hadde dette?

Kjøper at det er lettere å få et cluster som er riktigere konfigurert da det sannsynligvis er få som fanger opp at det skjer unødvendig scraping. Men hva er ulempen motsatt? Blir det mye støy om man har satt opp scraping på noe som ikke lar seg scrape?

Jeg syns vi uansett bør fjerne spec.prometheus.enabled, slik at det holder at man angir hvilken path som skal scrapes (spec.prometheus.path). Hvis path angis, setter vi automatisk prometheus.io/scrape annotasjonen til true.

Sep 23 '19 10:09 jhrv

Det er ikke noe stort teknisk problem, men derfor vil man heller ikke få folk til å skru det av når man ikke har metrikker. Så jeg synes det er ryddigere at man har et bevisst forhold til dette og enabler det når man har metrikker man vil eksponere. Path er heller ikke standard og svært mange kjører med noe annet enn /metrics, men jeg er enig i at det burde være nok å oppgi denne pathen for å enable scraping.

Sep 24 '19 05:09 terjesannum

It sounds like the consensus is to leave the default behavior as it is, but change the semantics so that:

.prometheus.enabled is removed
.prometheus.path will not have a default anymore
setting .prometheus.path explicitly will enable scraping, a blank value will disable scraping

Executing this change will result in scraping being enabled for existing applications that specify .prometheus.enabled = false, because the metrics path is set by default. The next application deployment will disable scraping again.

/cc @jhrv @terjesannum @Kyrremann

Sep 25 '19 12:09 kimtore

Jeg syns det er dumt at vi skal endre til at folk må skrive inn path, gjør hele opplegget mer komplisert. Jeg tenker heller at man bruker enabled, så får du default oppsett, og de som vil kan bare bruke path, og trenger da ikke sette enabled.

Sep 26 '19 06:09 Kyrremann

That sounds confusing. You turn the feature on either setting path or enabled? If it's not auto-enabled by setting path, it should be set explicitly. This makes it easier to document and understand, and makes this issue a no-op.

Sep 26 '19 07:09 kimtore

I checked one of the clusters, and a lot of the pods does not use /metrics as metrics path. So I think that specifying a metrics path is the best option to enable scraping.

setting .prometheus.path explicitly will enable scraping, a blank value will disable scraping

k get pod -o 'jsonpath={..metadata.annotations.prometheus\.io/path}' | tr ' ' '\n' | sort | uniq -c | sort -n
...
      3 /actuator/metrics
      6 /api/actuator/prometheus
     14 internal/metrics
     18 /
     20 /internal/metrics
     26 internal/prometheus
     27 actuator/prometheus
     40 /actuator/prometheus
     73 /internal/prometheus
     81 /prometheus
    264 /metrics

Sep 26 '19 11:09 terjesannum

Vi har valgt å ikke merge denne inn, da vi heller ønsker å vente til flere features som ikke er bakoverkompatible skal rulles ut. Dette for å minske jobben til utviklere, og at dette er mer kjekt å ha enn nødvendig.

Dec 11 '19 09:12 Kyrremann