Add revision to the dashboard title of the Grafana #510
Conversation
With this change, two different OpenSearch applications, in different revisions, will have separate dashboards when related to COS. - data-platform-helpers was added to use the get_charm_revision - titles of dashboards are changed during the charm upgrade hook - config_changed was added on refresh_events of COSAgentProvider to re-render the dashboards after a charm upgrade
gabrielcocenza
requested review from
zmraul,
phvalguima,
marcoppenheimer and
delgod
| @@ -244,6 +244,7 @@ def __init__(self, *args, distro: Type[OpenSearchDistribution] = None): | |||
| metrics_endpoints=[], | |||
| scrape_configs=self._scrape_config, | |||
| refresh_events=[ | |||
| self.on.config_changed, | |||
There was a problem hiding this comment.
question: Why is this one needed?
There was a problem hiding this comment.
The COSAgentProvider has self.on.config_changed by default if no custom events are passed.
We are passing custom events, so there is no refresh on COS for config-changed.
When you upgrade a charm, after that the config-change hook is triggered.
In summary, I'm re adding the default behavior and at the same time cover my need of refreshing COS relation after charm upgrade
There was a problem hiding this comment.
Another thing...
I didn't add self.on_charm_upgrade because I was seeing a race condition. Talking with the COS team, the order when you add those handlers matters and COS Provider should in theory be added as a last handler.
To do that, we would need to move the COSAgentProvider to the last child class of the inheritance and I though that it would be simpler just add back the config-changed
There was a problem hiding this comment.
Shouldn't we refresh on COS broken event as well? It is a slightly off topic question for this PR but how can we assure the dashboard is gone if we remove the COS relation from opensearch?
There was a problem hiding this comment.
I've tested and as expected (at least for me), the dashboards are removed if the relation with grafana-agent is removed
| @@ -228,3 +231,44 @@ def mask_sensitive_information(cmd: str) -> str: | |||
| pattern = re.compile(r"(-tspass\s+|-kspass\s+|-storepass\s+|-new\s+|pass:)(\S+)") | |||
|
|
|||
| return re.sub(pattern, r"\1" + "xxx", cmd) | |||
|
|
|||
|
|
|||
| def update_grafana_dashboards_titles(charm: "OpenSearchBaseCharm") -> None: | |||
There was a problem hiding this comment.
Please move this to a separate helper helper_cos
| for dashboard_path in path.iterdir(): | ||
| if dashboard_path.is_file() and dashboard_path.suffix == ".json": |
There was a problem hiding this comment.
why not directly target opensearch.json?
There was a problem hiding this comment.
I guess it will never happen, but I was thinking that maybe the file can change or maybe in the future we can have more than one file. In such scenario the code would still work. Maybe is not necessary. What do you think? Should I make it simple and target opensearch.json directly?
| except (json.JSONDecodeError, IOError) as e: | ||
| logger.error("Failed to process %s: %s", dashboard_path.name, str(e)) | ||
| else: | ||
| logger.warning("%s is not a valid JSON file", dashboard_path.name) |
There was a problem hiding this comment.
I don't think we need to handle these
| old_title = dashboard.get("title") | ||
| if old_title: |
There was a problem hiding this comment.
nit:
| old_title = dashboard.get("title") | |
| if old_title: | |
| if old_title := dashboard.get("title"): |
I believe we can always assume dashboards have a title and no need to test on it, otherwise it's a bug
With this change, two different OpenSearch applications in different revisions, will have separate dashboards when related to COS.
How to test it