Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore(rfcs): Add RFC for Apache HTTP Server metrics source #3519
chore(rfcs): Add RFC for Apache HTTP Server metrics source #3519
Changes from 4 commits
b7c3c1c
fd12446
186c1d8
473f128
995d81d
d87d29f
e3e9560
39aaa65
1a07528
96a9412
d0922b3
7426b37
7833587
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have this metric listed above. Do you still want to keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one is actually different, I believe it is just the CPU load of the httpd process as opposed to the entire system load (which is also available via
mod_status
). I think the general system load would be better handled by thehost_metrics
source.I wish they had better docs for
mod_status
, but that was my interpretation based on experiments and looking at https://github.com/apache/httpd/blob/0a172a81ec8b9d280349da1dec455a2dba287ace/modules/generators/mod_status.c#L476-L508There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct from memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(perhaps rename this to
System Load
to make this less ambiguous)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the big missing piece here is Basic auth - a lot of people still use this as security for mod_status. Prom just adds it to the scrape URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I think we could support this in the same way (via the URL), but that it'd be useful to add basic auth options for all HTTP-based client sources. I propose deferring that until after we refactor them as mentioned in "Future Work".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - missed that. +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree with this.
I can also see us (at some point) having "aliased" components, that merely defer to some other generic component with a default set of configurations.
In this case, we'd have a
http_scrape
source that is highly configurable, and aapache
alias source, that makes it more discoverable and easier to configure but defers its actual operations to thehttp_scrape
source.Having said that; all of this is irrelevant for this initial source implementation, which I like 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is actually what you described in "Future Work", isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The future work is just to do this internally, but I do like your idea of eventually adding an
http_scrape
source that would be more flexible and allow people to scrape arbitrary endpoints. I imagine it might require more configuration which the "aliases" would simply default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this currently. Sources and sinks can wrap other sources and sinks. For example, the
syslog
source wraps thesocket
source.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this precedence, do we want to create a generic
http_scrape
source and theapache
source based on this RFC, or do we want to start with the latter, and transition it to the former as soon as we need another source based on HTTP scraping?I assume we'll just start with the singular
apache
source, but want to make sure we're on the same page.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to defer the
http_scrape
source as I think it'll require more thought and discussion about how to parse the incoming requests (formats? codecs? etc.).I guess I figured I'd start with this one and the nginx one (#3091) for now and refactor them internally to share a component (along with the
prometheus
source).