ROX-28151: Custom Prometheus metrics minimal configuration#15783
ROX-28151: Custom Prometheus metrics minimal configuration#15783parametalol wants to merge 1 commit intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at a91a09a. To use with deploy scripts, first |
|
Images are ready for the commit at b434114. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15783 +/- ##
=======================================
Coverage 49.07% 49.08%
=======================================
Files 2640 2642 +2
Lines 195735 195747 +12
=======================================
+ Hits 96059 96081 +22
+ Misses 92160 92159 -1
+ Partials 7516 7507 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
parametalol
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @parametalol - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
vikin91
left a comment
vikin91
left a comment
There was a problem hiding this comment.
This size of PR is definitely easier to review!
Finding scope of this PR
I have followed the business logic and identified the following flow:
- We add new items to the configuration and the configuration object arrives over the API
- We validate that object and convert it to another object (
TranslateMetricLabels) - If the validation fails, we reject the object provided in the API call.
This scope is limited to a single flow and easy to follow.
Observations
However, I can clearly see that there are other construct in this PR that are part of a bigger implementation, but are not necessary for the flow that this PR represents. Many things are introduced without explaining the "why" behind them (mainly the type aliases, file/pkg naming and structure). Moreover, there are many entities introduced (named with nouns) without explanation what they are doing and why they are needed (example: finding, getter, aggregator, tracker). It is hard to navigate the code when the reader does not understand what those things do.
In order to understand how those things will be composed into a feature, they must be explained when they are introduced. I would either suggest to add this to this PR, or remove all things that are not needed here.
(I checked the entire feature code and selected unit tests).
|
I apologize that you waited so long for my review. After a short conversation with Alex, I think it would be better if someone else would review this feature. |
stehessel
left a comment
stehessel
left a comment
There was a problem hiding this comment.
Thank you for refactoring and minimalizing your previous PR. Overall I'm happy with the configuration part, but I would appreciate some more comments on WHY things are done as they are - both on the proto file, and whenever new entities or concepts are introduced.
parametalol
force-pushed
the
michael/ROX-28151-minimal-configuration
branch
from
841e4a0 to
25f4739
Compare
parametalol
force-pushed
the
michael/ROX-28151-minimal-configuration
branch
from
25f4739 to
a91a09a
Compare
|
Merged within #15797. |
5 participants
Description
This PR adds a new property to the private section of the central configuration storage object. It is accessible via the
/v1/configAPI, and configurable via the System Configuration UI page.This is the initial change to support further feature implementation in the following PRs.
Default state (empty configuration) doesn't enable any gathering.
Design document.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Unit tests.
Current dependencies on/for this PR: