feat: add linux agent data into metrics-cache - #24
Conversation
2783cc5 to
fa04ca7
Compare
| pub reader_allowlist: Vec<String>, | ||
| pub writer_allowlist: Vec<String>, | ||
| pub kubelet_stats_summary_cache: Cache<String, Arc<MetricsFetcherIngestion<StatsSummary>>>, | ||
| pub linux_agent_cache: Cache<String, Arc<String>>, |
There was a problem hiding this comment.
Consider Cache<String, Arc<MetricsFetcherIngestion<String>>
Or better, newtype the String and have Cache<String, MetricsFetcherIngestion<LinuxAgentOutput>>
You can follow the kubelet stats ingestion as an example - the advantage here is that we can inject the timestamp of the last received payload and use it for self-health reporting (for example, warn if we haven't gotten a agent payload in X minutes).
There was a problem hiding this comment.
Actually (this goes with my comment below, about non-utf-8 output), I'd go even further here and use Bytes instead of String to be safe. Otherwise "verbatim" is questionable in the doc comment in ingest.rs 😅
| const AGENT_PATH: &str = "/usr/local/bin/check_mk_agent"; | ||
| const AGENT_TIMEOUT: Duration = Duration::from_secs(5); |
There was a problem hiding this comment.
Can be later, but maybe eventually we should take both of these from CLI args and drop them into the helm chart. Definitely the timeout.
The agent path I could be convinced either way, it ties this code to how our image is constructed but realistically if someone is using it, they will be using our image (and if they "fork" it and want to run a different agent, they could just overwrite /usr/local/bin/check_mk_agent in their custom image).
| let kubelet_scrape = tokio::spawn(kubelet_stats_summary_scraper.loop_push_scrape()); | ||
| let _ = tokio::try_join!(kubelet_scrape); | ||
| let linux_agent_scrape = tokio::spawn(linux_agent_scraper.loop_push_scrape()); | ||
| let _ = tokio::try_join!(kubelet_scrape, linux_agent_scrape); |
There was a problem hiding this comment.
I would consider tokio::select! here (see metrics-cache/src/main.rs for an example).
Otherwise right now (also in the pre-existing code) we'll exit 0 with no useful output at all, if somehow we panic in either loop. I'd copy the select! + log + bail pattern from metrics-cache main().
There was a problem hiding this comment.
Nice little trick. Needed to inform myself about tokio::select!
| use crate::handlers::app; | ||
| use crate::state::tests::{MockValidator, test_app_state_with_validator}; | ||
|
|
||
| fn no_pull_agent() -> PullAgentMiddlewareConfig { |
There was a problem hiding this comment.
If you wanted instead of this I'd also be okay with a (manual) Default instance for PullAgentMiddlewareConfig that sets auth_enabled: true, shared_secret: None (this way if the default ever somehow gets used in non-test code, it defaults closed).
Thoughts?
| fn content_type(&self) -> &'static str { | ||
| match self { | ||
| Self::KubeletStatsSummary(_) => "application/json", | ||
| Self::CheckmkLinuxAgent { .. } => "text/plain; charset=utf-8", |
There was a problem hiding this comment.
Do we know that it's utf-8? I wonder if there are cases (particularly I am thinking of "someone patches our image to add a plugin") where the agent might produce non-utf-8 output?
There was a problem hiding this comment.
You are right, we do not 100% know that it's utf-8. This should be safer.
| pub reader_allowlist: Vec<String>, | ||
| pub writer_allowlist: Vec<String>, | ||
| pub kubelet_stats_summary_cache: Cache<String, Arc<MetricsFetcherIngestion<StatsSummary>>>, | ||
| pub linux_agent_cache: Cache<String, Arc<String>>, |
There was a problem hiding this comment.
Actually (this goes with my comment below, about non-utf-8 output), I'd go even further here and use Bytes instead of String to be safe. Otherwise "verbatim" is questionable in the doc comment in ingest.rs 😅
|
Some thoughts inline, but I agree with the general shape of this. Nice work! 🙂 |
b052b7c to
c528387
Compare
relrod
left a comment
There was a problem hiding this comment.
Generally looks good, just a few minor tweaks, then I think this is good to go!
- linux_agent_cache stores MetricsFetcherIngestion<Bytes> instead of a bare String/Arc<String>, matching kubelet_stats_summary_cache and giving self-health reporting a received_at timestamp to use later - switch the ingest handler and cache to Bytes throughout, since check_mk_agent plugin output isn't guaranteed to be valid UTF-8; drop the false "charset=utf-8" claim on the content-type header sent by metrics-fetcher - simplify NODE_NAME lookup and agent-timeout handling in metrics-fetcher's LinuxAgentScraper per suggested diffs - fix metrics-fetcher main() silently exiting 0 if either scrape loop panics, by select!+log+bail on the join handles like metrics-cache's main() does - move the linux_agent ingest router tests into handlers/mod.rs, since they exercise routing/middleware rather than the handler itself; add a manual, fail-closed Default for PullAgentMiddlewareConfig instead of an ad-hoc no_pull_agent() test helper - drop the payload.rs unit tests that only asserted match arms
c528387 to
a3a33fb
Compare
| use super::*; | ||
| use crate::state::tests::test_app_state; | ||
|
|
||
| /// Exercises the handler directly (no router, no auth middleware) — this |
|
Also: Note, we'll want to snapshot the cache at snapshot creation time when you work on the rendering part. Otherwise it could change out from under us as we're rendering sections. |
CMK-36613