prometheus: add server setup documentation #2419
Conversation
|
LGTM |
doc/prometheus.md
Outdated
| 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(); |
There was a problem hiding this comment.
i'd suggest, instead of putting this in the document, just add a proper doyxgen document for
.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().
There was a problem hiding this comment.
Removed error handling.
If you prefer doxygen:
- I think it should be at the header level because it aims to present complete examples.
- 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.
There was a problem hiding this comment.
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/*.mdfiles?
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.mdcontain only a link to doxygen, or have a pure link.
that's a different story. again, i think, they serve for different purposes.
There was a problem hiding this comment.
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
force-pushed
the
prometeheus-add-server-setup-doc
branch
from
a635efa
to
211bd1e
Compare
|
@xemul Can we merge this? |
Add missing Prometheus server setup documentation to
doc/prometheus.md.