Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add revision to the dashboard title of the Grafana #510

Open
wants to merge 1 commit into
base: 2/edge
Choose a base branch
from

Conversation

gabrielcocenza
Copy link
Member

@gabrielcocenza gabrielcocenza commented Dec 2, 2024

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

How to test it

  1. Deploy more than one opensearch application
  2. Relate both apps with grafana-agent
  3. In one of the applications use juju refresh and apply a local build of this patch
  4. Go to the Grafana dashboards an you will see that two dashboards are created:
  • one with no revision (from charmhub)
  • one with Rev 0 (meaning that is a local build)

image

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
@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Why is this one needed?

Copy link
Member Author

@gabrielcocenza gabrielcocenza Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested and as expected (at least for me), the dashboards are removed if the relation with grafana-agent is removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants