search

PromPHP / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
437 stars 96 forks source link

On flush remove only prometheus specific keys from redis #31

Closed encero closed 3 years ago

encero commented 3 years ago

I am reasonably sure this change will don't break anyone's code.

In our company workflow flushing entire redis ( all databases ) when cleaning up stale metrics, would be really nasty surprise.

When i was changing test setup code, it dawned on me that those flush* methods wasn't probably meant for public API at all.

In the event of not accepting this PR, i humbly suggest changing the flushing mechanism from flushAll to flushDB, limiting the potential blast radius.

LKaemmerling commented 3 years ago

@encero thank you! For me it looks fine. @rdohms can you please have a look too?

rdohms commented 3 years ago

Trying to grok the change here. Should we not try to switch this to clean only our own metrics? And the. Still keep the cleanup on start?

Bit out of the loop on the specifics.

LKaemmerling commented 3 years ago

@rdohms basically we clean the whole redis database at the moment. This MR changes the behavior to only drop all keys generated by this lib. The question would be, makes it sense to do this? Should the method be Public?

I think: the method should be accessible and only clean the keys the library generates.

encero commented 3 years ago

The failing phpstan check should be fixed in upstream as its unrelated to this PR.

LKaemmerling commented 3 years ago

Good job, thank you! Looks fine for me. I personally don't want to release a new version right before the holidays, so we should merge it next year

LKaemmerling commented 3 years ago

FYI, the phpstan error will be fixed with https://github.com/PromPHP/prometheus_client_php/pull/35

encero commented 3 years ago

The branch is rebased on current upstream. All checks green. So if there aren't any further question. I will leave this PR to your discretion.

encero commented 3 years ago

I just found one small typo. Could you please fix it :)? Otherwise, it looks okay for me.

Fixed, nice catch with that typo, I am wondering how i was able to introduce it in the first place 🤔

LKaemmerling commented 3 years ago

Good job @encero! Thank you!