Conversation
Add configurable network and observer identities to the network observer to fully qualify metrics. Signed-off-by: Christian Kruse <christian@c-kruse.com>
Restrict the internal prometheus proxy endpoint to relevant metrics. Normalizes the http requests, enforces an allow-list of metric names and constrains all selectors to the observer's configured network_id. Signed-off-by: Christian Kruse <christian@c-kruse.com>
Pass the namespace as the network ID so observers sharing the host Prometheus use separate metric partitions. Signed-off-by: Christian Kruse <christian@c-kruse.com>
Render the network-id argument when networkId is set. Document its purpose for users collecting metrics. Signed-off-by: Christian Kruse <christian@c-kruse.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe network observer now labels metrics with network and observer identities. Its Prometheus proxy validates, allow-lists, and network-scopes queries. Helm and non-Kubernetes deployments pass the network ID to the observer. ChangesNetwork observer metric isolation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Console
participant NetworkObserver
participant CommonPrometheus
Console->>NetworkObserver: send Prometheus query
NetworkObserver->>NetworkObserver: validate and add network_id matcher
NetworkObserver->>CommonPrometheus: forward scoped query
CommonPrometheus-->>NetworkObserver: return results
NetworkObserver-->>Console: return response
Merge Risk: ⚪ Minimal · up to The console’s current Prometheus requests remain compatible with the new network-scoping policy, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 11 files. (4 skipped: 4 unsupported.)
Comment |
| Name: fmt.Sprintf("%s-skupper-network-observer", namespace), | ||
| Image: images.GetNetworkObserverImageName(), | ||
| Command: []string{ | ||
| fmt.Sprintf("-network-id=%s", namespace), |
There was a problem hiding this comment.
I wonder if this is necessary, currently, targets defined in prometheus are labeled with their namespace:
~$ curl -s http://localhost:9091/api/v1/targets | jq .
{
"status": "success",
"data": {
"activeTargets": [
{
"discoveredLabels": {
"__address__": "localhost:9000",
"__meta_filepath": "/etc/prometheus/targets/east.json",
"__metrics_path__": "/metrics",
"__scheme__": "http",
"__scrape_interval__": "15s",
"__scrape_timeout__": "10s",
"job": "skupper-network-observers",
"namespace": "east"
},
"labels": {
"instance": "localhost:9000",
"job": "skupper-network-observers",
"namespace": "east"
},
"scrapePool": "skupper-network-observers",
"scrapeUrl": "http://localhost:9000/metrics",
"globalUrl": "http://li-12aec04c-3404-11b2-a85c-c8b28bd65981.ibm.com:9000/metrics",
"lastError": "",
"lastScrape": "2026-09-18T19:42:10.665199492Z",
"lastScrapeDuration": 0.00286493,
"health": "up",
"scrapeInterval": "15s",
"scrapeTimeout": "10s"
},
{
"discoveredLabels": {
"__address__": "localhost:9001",
"__meta_filepath": "/etc/prometheus/targets/west.json",
"__metrics_path__": "/metrics",
"__scheme__": "http",
"__scrape_interval__": "15s",
"__scrape_timeout__": "10s",
"job": "skupper-network-observers",
"namespace": "west"
},
"labels": {
"instance": "localhost:9001",
"job": "skupper-network-observers",
"namespace": "west"
},
"scrapePool": "skupper-network-observers",
"scrapeUrl": "http://localhost:9001/metrics",
"globalUrl": "http://li-12aec04c-3404-11b2-a85c-c8b28bd65981.ibm.com:9001/metrics",
"lastError": "",
"lastScrape": "0001-01-01T00:00:00Z",
"lastScrapeDuration": 0,
"health": "unknown",
"scrapeInterval": "15s",
"scrapeTimeout": "10s"
}
],
"droppedTargets": [],
"droppedTargetCounts": {
"skupper-network-observers": 0
}
}
}
~$ curl -s 'http://localhost:9091/api/v1/query?query=up' | jq '.data.result'
[
{
"metric": {
"__name__": "up",
"instance": "localhost:9000",
"job": "skupper-network-observers",
"namespace": "east"
},
"value": [
1789760604.794,
"1"
]
},
{
"metric": {
"__name__": "up",
"instance": "localhost:9001",
"job": "skupper-network-observers",
"namespace": "west"
},
"value": [
1789760604.794,
"1"
]
}
]
There was a problem hiding this comment.
@nluaces at least in this iteration we do need it. I recall that our original plan involved the namespace label. It was a two-step plan: inject a namespace label in the scrape configuration and then filter metrics on that label for the console's queries.
The idea behind adding a network-id to the network-observers on system sites is that it accomplishes both pieces with a single flag: partition metrics from different observers in the database, then automatically filter console queries down to the right partition. The original namespace label plan would work just as well for system sites. For kube deployments where the user decides to centrally collect metrics (system sites too, I suppose) I think it is better to prescribe a scheme (network_id, observer_id) rather than leaving it up to the user to pick how they want to partition and query metrics from many observers.
That said, keeping both the namespace and the network-id labels aught to be fine. It may even be nice as you've demonstrated, it adds context to the targets: localhost:9000 doesn't mean much to me without tracking it down running netstat, but namespace=east is a good hint.
Does that make sense?
2 participants
Resolves #2566
Summary by CodeRabbit
New Features
Bug Fixes
Documentation