Conversation
reneradoi
left a comment
reneradoi
left a comment
There was a problem hiding this comment.
Minor comments/questions. Looks good so far, very nice integration tests!
| dns_ip = subprocess.check_output(cmd, shell=True).decode().strip().strip("'\"") | ||
| except Exception: | ||
| dns_ip = "" | ||
| dns_ip = dns_ip or "10.152.183.10" |
There was a problem hiding this comment.
What magic ip address is this? Can you please add a comment?
There was a problem hiding this comment.
It is the default dns address on microk8s
| logger.info("Waiting for applications to settle after K8s COS integration") | ||
| juju.wait( | ||
| lambda s: ( | ||
| s.apps[APP_NAME].app_status.current == "active" |
There was a problem hiding this comment.
Why not are_apps_active_and_agents_idle?
There was a problem hiding this comment.
OTELCOL stays blocked until it is fully integrated into a backend. That is why I only used active.
| valkey_unit = f"{APP_NAME}/0" | ||
| logger.info("Verifying cos-agent relation data for %s", valkey_unit) | ||
| raw_config = None | ||
| for attempt in Retrying(stop=stop_after_delay(180), wait=wait_fixed(5)): |
There was a problem hiding this comment.
Why 3 minutes here and only 2 in the K8s test?
There was a problem hiding this comment.
On VM it raised a couple of times for me. I will unify the tests to 180s.
| } | ||
| ] | ||
| }, | ||
| "description": "Redis Dashboard for Prometheus Redis Exporter 1.x", |
There was a problem hiding this comment.
nit: I know we can't change the metrics themselves, but let's change at least the description from Redis to Valkey.
| password = self.state.cluster.internal_users_credentials.get( | ||
| CharmUsers.VALKEY_MONITORING.value | ||
| ) | ||
| if not password: | ||
| return | ||
|
|
There was a problem hiding this comment.
nit: I believe this is duplicate, as one line below we get env = self.exporter_env() and we could do if not env.get(CharmUsers.VALKEY_MONITORING.value): return
| return False | ||
|
|
||
| self.container.add_layer(CHARM, self.pebble_layer, combine=True) | ||
| self.container.replan() |
There was a problem hiding this comment.
I'm confused why we replan here (which should restart changed services) and restart the service in the caller method in the manager class - I believe these are duplicate restarts.
2 participants
Integrate the Canonical Observability Stack (COS) for Valkey on both Kubernetes and VM substrates.
metrics-endpoint,grafana-dashboard, andloggingrelation endpoints usingMetricsEndpointProvider,GrafanaDashboardProvider, andLogProxyConsumer.cos-agentsubordinate relation usingCOSAgentProvider(vendoringcharms.grafana_agent.v0.cos_agentv0.27) to configure Prometheus metrics scrape endpoints, Loki log targets, and dashboard files.cos-agent.tests/integration/observability/), and Spread task definitions (tests/spread/{k8s,vm}/test_telemetry.py).Verification
tox run -e lintpassed.tox run -e staticpassed with 0 errors.tox run -e unitpassed (100% coverage onsrc/events/observability.py).K8s Dashboard