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

Move some common objects to observability-libs #24

Open
sed-i opened this issue Aug 19, 2022 · 5 comments
Open

Move some common objects to observability-libs #24

sed-i opened this issue Aug 19, 2022 · 5 comments

Comments

@sed-i
Copy link
Contributor

sed-i commented Aug 19, 2022

Enhancement Proposal

class AlertRules

AlertRules is used in both prometheus_scrape and remote_write. A variation also exists in loki_push_api.

class CosTool

CosTool is used in prometheus_scrape, remote_write and loki_push_api.

_type_convert_stored

_type_convert_stored is used in cos-configuration-k8s, alertmanager-k8s, loki-k8s, avalanche-k8s, grafana-agent-k8s, grafana-k8s, traefik-k8s, prometheus-k8s, cassandra-k8s.

Unless OF team would be willing to accept this somewhere.

relation direction validation

All of our charms either do it, or should do it, and the code definitely is generic enough to fit as a lib. One thing to keep in mind there however is that it needs to be a scoped import, as we don't want to impose the dependency on anyone else.
-- @simskij

@rbarry82
Copy link
Contributor

OF team has already rejected the essential premise of _type_convert_stored (the entire method came out of a comment trying to demonstrate the need for it, and the relative simplicity of resolving it).

In general, I'd ask:

  • How many charms actually need _type_convert_stored? We should check this first. Grafana does, but that's mostly "legacy" at this point, since at present (in edge and most of the recent versions), any complex objects are kept in peer data to avoid this. traefik-k8s may, eventually, unless Pietro makes the same tradeoff.
  • AlertRules should be carefully considered. Loki's variation is actually due to Loki alert rules not being a 1:1 structural mapping to Prometheus rules. If we were to absorb it here, we'd need to very carefully consider the design and how to override parts of it.

@simskij
Copy link
Member

simskij commented Aug 23, 2022

I think the highest ranking logic to make a lib out of at this point would be the relation direction validation. All of our charms either do it, or should do it, and the code definitely is generic enough to fit as a lib. One thing to keep in mind there however is that it needs to be a scoped import, as we don't want to impose the dependency on anyone else.

@simskij
Copy link
Member

simskij commented Aug 24, 2022

Let's put this on the backburner until after we've gone GA.

@sed-i
Copy link
Contributor Author

sed-i commented Aug 29, 2022

Since (almost?) all charms already import juju_topology, maybe we could rename the lib from juju_topology to something else and ...

@simskij
Copy link
Member

simskij commented Aug 30, 2022

Since (almost?) all charms already import juju_topology, maybe we could rename the lib from juju_topology to something else and ...

And? :D

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

No branches or pull requests

3 participants