Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(metrics): Add rest of master chartdata metrics #152

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

uristdwarf
Copy link
Collaborator

This commit finishes adding metric data from master chartsdata. Since all the chartsdata was counters in Prometheus terminology, there was no need to add other types, however a a small refactor was needed to accomodate master metrics and in the future other service metrics. Notably removed was CPU usage and memory, since there are better ways to gather this data, including with a prometheus server itself (which will scrape this data).

This commit deprecates master chartsdata, and is the first step in deprecating the CGI monitoring

lgsilva3087
lgsilva3087 previously approved these changes Jul 23, 2024
Copy link
Contributor

@lgsilva3087 lgsilva3087 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I see you are almost not using anymore #include "common/platform.h". I don't remember the last decision about it, can you please remaind it.

ralcolea
ralcolea previously approved these changes Jul 26, 2024
Copy link
Contributor

@ralcolea ralcolea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good job @uristdwarf 👍 🔥 🚀

I added some minor suggestions to improve readability and other aspects, but they are not stoppers for merging.

@uristdwarf
Copy link
Collaborator Author

LGTM. I see you are almost not using anymore #include "common/platform.h". I don't remember the last decision about it, can you please remaind it.

For new libraries, I think we are not adding it but for existing/refactored ones we are

ralcolea
ralcolea previously approved these changes Oct 22, 2024
Copy link
Contributor

@ralcolea ralcolea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 🔥 🚀
The only suggestion I have before merging (if not done) is to run test_prometheus.sh with ENABLE_PROMETHEUS = 0 to confirm everything works as expected.

Copy link
Contributor

@lgsilva3087 lgsilva3087 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes about bytes sent and received. The call to increment should use the second argument (by default it is 1).

@lgsilva3087
Copy link
Contributor

LGTM. I see you are almost not using anymore #include "common/platform.h". I don't remember the last decision about it, can you please remaind it.

For new libraries, I think we are not adding it but for existing/refactored ones we are

Maybe this kind of decisions should be communicated to all devs.

lgsilva3087
lgsilva3087 previously approved these changes Dec 3, 2024
Copy link
Contributor

@lgsilva3087 lgsilva3087 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@uristdwarf uristdwarf force-pushed the prometheus-two branch 2 times, most recently from 1a87bce to da455ba Compare December 9, 2024 16:14
ralcolea
ralcolea previously approved these changes Dec 10, 2024
Copy link
Contributor

@ralcolea ralcolea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @uristdwarf 👍 🔥 🚀
I shared some minor suggestions for your consideration, but none of them are stoppers to merge.

However, I had compilation errors when building the project without installing prometheus-cpp-dev package. Can you confirm whether this scenario works as expected?

@uristdwarf uristdwarf dismissed stale reviews from ralcolea and lgsilva3087 via 83fd218 December 12, 2024 08:26
@uristdwarf uristdwarf force-pushed the prometheus-two branch 4 times, most recently from c2e4813 to 3a94d51 Compare December 13, 2024 13:59
ralcolea
ralcolea previously approved these changes Dec 16, 2024
Copy link
Contributor

@ralcolea ralcolea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @uristdwarf 👍 🔥 🚀

This commit finishes adding metric data from master chartsdata. Since
all the chartsdata was counters in Prometheus terminology, there was no
need to add other types, however a a small refactor was needed to
accomodate master metrics and in the future other service metrics.
Notably removed was CPU usage and memory, since there are better ways to
gather this data, including with a prometheus server itself (which will
scrape this data).

This commit deprecates master chartsdata, and is the first step in
deprecating the CGI monitoring

Signed-off-by: Urmas Rist <[email protected]>
Specifically, it checks that the master operations are working

Signed-off-by: Urmas Rist <[email protected]>
Previously it required you to have the library installed on the system.
Using FetchContent in CMake, it is now acquired externally if the CMake
config does not exist on the system and the build option is correctly set
(which it is by default). This will statically compile the library into
the binary.

Signed-off-by: Urmas Rist <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants