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

utl: Adds live prometheus monitoring to OpenROAD #6741

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

Conversation

QuantamHD
Copy link
Collaborator

OpenROAD now includes a metrics endpoint server that can track internal tool metrics over time.

To use this feature you need to do the following start the prometheus and grafana collectors

image

Detailed instructions:

$ cd etc/monitoring
$ docker compose up -d

This will start a grafana endpoint ready to collect from the OpenROAD application you would like to track. By default it's looking for an http server running on port 8080 on your localhost.

To start the metrics endpoint in OpenROAD, run:

utl::startPrometheusEndpoint 8080

This is all configurable in the docker compose file, and you should be able to access grafana by going to http://localhost:3000 username: admin, password: grafana. Go to the dashboard tab and click service, then OpenROAD to see the pre-made dashboard.

OpenROAD includes a metrics endpoint server that can track internal tool metrics over time.

To use this feature you need to do the following start the prometheus and grafana collectors

[Detailed instructions](/etc/monitoring/README.md):
```shell
$ cd etc/monitoring
$ docker compose up -d
```

This will start a grafana endpoint ready to collect from the OpenROAD application you would
like to track. By default it's looking for an http server running on port 8080 on your localhost.

To start the metrics endpoint in OpenROAD, run:
```tcl
utl::startPrometheusEndpoint 8080
```

This is all configurable in the docker compose file, and you should be able to access grafana by going to
http://localhost:3000 username: admin, password: grafana. Go to the dashboard tab and click service,
then OpenROAD to see the pre-made dashboard.

Signed-off-by: Ethan Mahintorabi <[email protected]>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 51. Check the log or trigger a new build to see more.

@maliberty
Copy link
Member

This appears to have a copy of https://github.com/jupp0r/prometheus-cpp. It should be in third-party.

@QuantamHD
Copy link
Collaborator Author

QuantamHD commented Feb 20, 2025

This is a proper fork. I think it should live in utl. I renamed all the namespaces and redid the imports so it could be a proper part of utl.

@maliberty
Copy link
Member

Is there a reason we want a fork? Will we never want upstream updates?

@QuantamHD
Copy link
Collaborator Author

QuantamHD commented Feb 20, 2025

The library hasn't been updated in two years and is arguably complete. At the end of the day it's essentially just a map with some keys and values and a way to serialize it into a text file.

I have more ideas on how to drive it forward that are good for OpenROAD, but arguably not in the interest of the original library. Like adding more pluggable inheritance and a way to stub in Google's monitoring backend Monarch.

Even in this version I removed its dependency on its original http library. Moved to pull vs push, and implemented the http stuff based on boost

@QuantamHD
Copy link
Collaborator Author

Also this is the original implementation not the one you linked to. https://github.com/biaks/prometheus-cpp-lite

@maliberty
Copy link
Member

The one I pointed probably started as a copy but has more recent commits and many more stars and forks. It seems like the original died but this one is still alive fwiw.

@QuantamHD
Copy link
Collaborator Author

That's the official otel one. We just don't really need what it provides, namely a bunch of http server stuff, and unnecessary performance cruft. It's better to just have our own fork that we have full control over. Performance stuff matters in webservers where it could conceivably add to response latency, but we're talking 100s of milliseconds at worst, and when you're talking about OpenROAD we're talking hours of runtime against negligible metrics cost.

Comment on lines -39 to +68
| Switch Name | Description |
| ----- | ----- |
| `name` | Name of the command/message to query. |
| `-manpath` | Include optional path to man pages (e.g. ~/OpenROAD/docs/cat). |
| Switch Name | Description |
| ----------- | ------------------------------------------------------------------------------------------------------------------------------------ |
| `name` | Name of the command/message to query. |
| `-manpath` | Include optional path to man pages (e.g. ~/OpenROAD/docs/cat). |
Copy link
Member

Choose a reason for hiding this comment

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

Why is this reformatted with extraneous whitespace?

@@ -59,6 +59,9 @@

namespace utl {

class PrometheusMetricsServer;
class Registry;
Copy link
Member

Choose a reason for hiding this comment

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

Registry is a very generic name. I would prefer to see the code the prometheus subdir in namespace like utl::prometheus

Comment on lines +25 to +27
class PrometheusMetricsServer
{
std::thread worker_thread_;
Copy link
Member

Choose a reason for hiding this comment

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


public:
~PrometheusMetricsServer();
PrometheusMetricsServer(std::shared_ptr<Registry>& registry_,
Copy link
Member

Choose a reason for hiding this comment

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

worker_thread_.join();
}

std::string SnapshotPrometheusMetrics(Registry* registry)
Copy link
Member

Choose a reason for hiding this comment

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

local functions should be in an anonymous namespace https://google.github.io/styleguide/cppguide.html#Internal_Linkage

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.

2 participants