search

discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
525 stars 153 forks source link

Expand usage of MetricsContainer in other collectors #268

Closed sosedoff closed 1 year ago

sosedoff commented 1 year ago

Implements the MetricsContainer where applicable

sosedoff commented 1 year ago

@SamSaffron any thoughts? do we need extra tests?

SamSaffron commented 1 year ago

it looks great to me... but yeah I think we should have an integration test on each of these to ensure we did not regress.

Kind of slack we did not have the tests to start with ... process collector at least has some coverage.

Do you think I should hold off merging till we have better coverage everywhere or should we merge and revisit tests? I am open to either.. The refactor is great.

sosedoff commented 1 year ago

Yea i'd add extra tests before merging to prevent any potential breakage.

sosedoff commented 1 year ago

@SamSaffron please have a look around, leave feedback. Im back to wrap this PR up since it has a lot of fixes and improvements. I've got a few more tests to add but i think its already pretty solid as is.

SamSaffron commented 1 year ago

Nice this is looking quite tidy! finding it hard to find any things to change