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 1 commit
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.
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).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.
Yes. But I assume #2660 would solve this for both logs and metrics, so it's probably blocked on that. Agree?
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.
Hmm, I think that is just for internal metrics, yes? It looks like the prometheus source adds labels (looks like we call them tags):
https://github.com/timberio/vector/blob/61e806d01d4cc6d2a527b52aa9388d4547f1ebc2/src/sources/prometheus/parser.rs#L43-L51
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.
Good point. I do think many of these event attributes would be better represented as trace context, but we can defer that for a later discussion.
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 prefer
httpd
orapache_httpd
? Apache feels a little generic since I think of the entire foundation. If we go with theapache_
prefix we should think aboutapache_kafka
and so on :/.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 had the same angst about using
apache
, but it does match how the other two collectors I looked at, datadog and telegraf, do it so people may be more familiar with that keyword as the component name if they are coming from those tools.I'm open to either of those options though. I guess I prefer
httpd
overapache_httpd
given thatkafka
is justkafka
and notapache_kafka
.