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

prometheus: add server setup documentation #2419

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomershafir
Copy link
Contributor

@tomershafir tomershafir commented Sep 4, 2024

Add missing Prometheus server setup documentation to doc/prometheus.md.

@tomershafir tomershafir marked this pull request as ready for review September 4, 2024 08:11
@xemul xemul requested a review from amnonh September 10, 2024 09:54
@amnonh
Copy link
Contributor

amnonh commented Sep 10, 2024

LGTM

Comment on lines 43 to 46
prometheus_server.listen(socket_address{addr, port}).handle_exception([addr, port] (auto e) {
std::cerr << seastar::format("Could not start Prometheus API server on {}:{}: {}\n", addr, port, e);
return make_exception_future<>(e);
}).get();
Copy link
Contributor

@tchaikov tchaikov Sep 13, 2024

Choose a reason for hiding this comment

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

i'd suggest, instead of putting this in the document, just add a proper doyxgen document for

future<> start(httpd::http_server_control& http_server, config ctx);
.

if we really want to highlight that start() is important to start the prometheus http request handling, please drop the error handling. because the point of the code list is for explaining that

prometheus::start(prometheus_server, pctx)

is necessary,the other part is for providing the context. not for showing how to write a proper seastar application. actually, to focus on the key concepts,
let's present only a relevant portion of the code rather than the entire application. this will prevent distractions and ensure that our explanation is clear and concise.

BTW, i'd suggest use fmt::print() directly here, instead of using seastar::format().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed error handling.

If you prefer doxygen:

  1. I think it should be at the header level because it aims to present complete examples.
  2. I think the entire document should be migrated.

But thats not so easy Q probably. Do you have feedback from users on docs? Do they like doc/*.md files? I think doxygen can be less discoverable in this case. Maybe we can move content and have prometheus.md contain only a link to doxygen, or have a pure link.

Copy link
Contributor

@tchaikov tchaikov Sep 13, 2024

Choose a reason for hiding this comment

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

Removed error handling.

If you prefer doxygen:

1. I think it should be at the header level because it aims to present complete examples.

i disagree. i think we should just highlight that what start() is for. the "complete" example is necessary if the document is not enough, and a code snippet is not good enough. but i think the document would suffice even without the code snippet.

2. I think the entire document should be migrated.

the document is served as a tutorial instead of a reference. they serve for different purposes

But thats not so easy Q probably. Do you have feedback from users on docs? Do they like doc/*.md files?

i don't have feedback from users apart from myself. as i am also a user of seastar. if i want to use an API, i read its header. if i am still confused, i read the document, demos and even tests, and its implementation for sure.

I think doxygen can be less discoverable in this case.

doxygen as a rendered document is not that discoverable as header is . but since we use doxygen to format the header document, probably we can continue using it. the content matters more than the format, IMHO.

Maybe we can move content and have prometheus.md contain only a link to doxygen, or have a pure link.

that's a different story. again, i think, they serve for different purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being a tutorial, I think a new user would read doc/prometheus.md and would not easily understand what header he should use and how to setup a prometheus server without this commit.

@tomershafir tomershafir force-pushed the prometeheus-add-server-setup-doc branch from a635efa to 211bd1e Compare September 13, 2024 06:53
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.

3 participants