Skip to content

Conversation

@zhuoyuan-liu
Copy link
Contributor

@zhuoyuan-liu zhuoyuan-liu commented Sep 27, 2024

This PR added Prometheus metrics support for osctrl-tls. Issue: #504

Currently, it has all default metrics go metrics such as memory info and gc duration etc. Also, we added one custom histogram metric to measure the total request/latency to each osquery endpoint.

The metric server would run as a separate service in a different port. We can query e.g. P90 latency for each endpoint with different environments with the histogram metric.

image

Also the request rate for different path and status code.
image

@zhuoyuan-liu zhuoyuan-liu marked this pull request as ready for review September 27, 2024 10:37
@javuto javuto self-assigned this Oct 1, 2024
@javuto javuto added osctrl-tls osctrl-tls related changes 📊 metrics Metrics related issues labels Oct 1, 2024
@javuto javuto self-requested a review October 2, 2024 17:24
@javuto
Copy link
Collaborator

javuto commented Oct 2, 2024

Thank you for the PR! I will add some comments in a code review 🙂

Copy link
Collaborator

@javuto javuto left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR! 🙏

I have added few comments, mostly related with having metrics not enabled by default, and the ability to enable them via configuration or flags.

@zhuoyuan-liu
Copy link
Contributor Author

I have updated the flag. Should I start to clean up the old metrics components for osctrl-tls?

@zhuoyuan-liu zhuoyuan-liu requested a review from javuto October 4, 2024 14:36
@javuto javuto mentioned this pull request Oct 8, 2024
@javuto
Copy link
Collaborator

javuto commented Oct 8, 2024

I have updated the flag. Should I start to clean up the old metrics components for osctrl-tls?

You mean the emitted metrics? Instead of removing them, it would be better if we can find a way for those to be utilized by prometheus too.

@zhuoyuan-liu
Copy link
Contributor Author

@javuto I have added the flag, could you please take another look?

@zhuoyuan-liu
Copy link
Contributor Author

@javuto I Just fixed the conflicts.

@javuto javuto merged commit 5ad3ce7 into jmpsec:main Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📊 metrics Metrics related issues osctrl-tls osctrl-tls related changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants