chore: ensure metrics are correctly exposed #701
Conversation
rust/operator-binary/src/crd/mod.rs
Outdated
| pub fn metrics_ports(&self, role: &HbaseRole) -> Vec<(String, u16)> { | ||
| match role { | ||
| HbaseRole::Master => vec![( | ||
| HBASE_METRICS_PORT_NAME.to_string(), | ||
| HBASE_MASTER_METRICS_PORT, | ||
| )], | ||
| HbaseRole::RegionServer => vec![( | ||
| HBASE_METRICS_PORT_NAME.to_string(), | ||
| HBASE_REGIONSERVER_METRICS_PORT, | ||
| )], | ||
| HbaseRole::RestServer => { | ||
| vec![(HBASE_METRICS_PORT_NAME.to_string(), HBASE_REST_METRICS_PORT)] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
note: The mapping of role to metrics port exists in two places: Here and below at line 575 which inevitably contains the risk to drift apart. I think it makes sense to contain this mapping in a single place instead.
One possible solution is to drop the associated metrics_port function and user .map() to extract only the port numbers when needed.
There was a problem hiding this comment.
Yeah this is a little redundant, will check how to clean it up.
| &rolegroup.role_group, | ||
| )) | ||
| .context(ObjectMetaSnafu)? | ||
| .with_label(Label::try_from(("prometheus.io/scrape", "true")).context(LabelBuildSnafu)?) |
There was a problem hiding this comment.
question: Does it make sense to pull this key out into a constant? Or make it an associated function on Label, like Label::prometheus_scrape()?
There was a problem hiding this comment.
I like that and did not think about that. This would mean touching every operator again... i could prepare it in operator-rs and maybe put it on a prelease bump list or something?
| fn prometheus_annotations(hbase: &v1alpha1::HbaseCluster, hbase_role: &HbaseRole) -> Annotations { | ||
| Annotations::try_from([ | ||
| ("prometheus.io/path".to_owned(), "/prometheus".to_owned()), | ||
| ( | ||
| "prometheus.io/port".to_owned(), | ||
| hbase.metrics_port(hbase_role).to_string(), | ||
| ), | ||
| ( | ||
| "prometheus.io/scheme".to_owned(), | ||
| if hbase.has_https_enabled() { | ||
| "https".to_owned() | ||
| } else { | ||
| "http".to_owned() | ||
| }, | ||
| ), | ||
| ("prometheus.io/scrape".to_owned(), "true".to_owned()), | ||
| ]) | ||
| .expect("should be valid annotations") | ||
| } |
There was a problem hiding this comment.
question: Similar question to above - does it make sense to pull these out into contants or associated function on Annotation?
There was a problem hiding this comment.
Same as above, i like the idea.
Techassi
moved this from Development: Waiting for Review
to Development: In Review
in Stackable Engineering
Co-authored-by: Techassi <[email protected]>
Co-authored-by: Techassi <[email protected]>
Co-authored-by: Techassi <[email protected]>
Description
Part of stackabletech/issues#747
This PR adds a metrics service with the additional Prometheus annotations. It also adds some documentation on monitoring for the TLS case since, similar to NiFi, HBase also exposes metrics behind a port which gets secured by TLS
Follow up monitoring Stack PR because moving to the metrics service, the port name changed: stackabletech/demos#316
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker