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

Switched to Loki log forwarding. #98

Merged
merged 1 commit into from
Sep 9, 2024
Merged

Switched to Loki log forwarding. #98

merged 1 commit into from
Sep 9, 2024

Conversation

pik4ez-canonical
Copy link
Contributor

@pik4ez-canonical pik4ez-canonical commented Sep 4, 2024

IAM-1029

Closes #97

@pik4ez-canonical pik4ez-canonical requested a review from a team as a code owner September 4, 2024 14:49
@pik4ez-canonical pik4ez-canonical changed the title Switched to Loki log forwarding. [WIP] Switched to Loki log forwarding. Sep 4, 2024
requirements.txt Outdated Show resolved Hide resolved
BarcoMasile
BarcoMasile previously approved these changes Sep 5, 2024
Copy link
Contributor

@BarcoMasile BarcoMasile left a comment

Choose a reason for hiding this comment

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

Great job!

Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

Good job, LGTM to me overall. I have a couple of small comments, once those are addressed and if you have manually tested it, we can merge it

src/charm.py Outdated Show resolved Hide resolved
tests/integration/test_charm.py Outdated Show resolved Hide resolved
@pik4ez-canonical pik4ez-canonical changed the title [WIP] Switched to Loki log forwarding. Switched to Loki log forwarding. Sep 9, 2024
@pik4ez-canonical
Copy link
Contributor Author

Thank you Marco and Nikos for your reviews. I made changes based on your feedback.

The only thing that's left and that's actually important is that alert rules aren't propagated to Loki and Grafana when using LogForwarder. The problem is in the logging library, see issue.

Nikos suggested to contact Observability team. But what would be your advice for this PR?

@BarcoMasile
Copy link
Contributor

BarcoMasile commented Sep 9, 2024

Thank you Marco and Nikos for your reviews. I made changes based on your feedback.

The only thing that's left and that's actually important is that alert rules aren't propagated to Loki and Grafana when using LogForwarder. The problem is in the logging library, see issue.

Nikos suggested to contact Observability team. But what would be your advice for this PR?

@pik4ez-canonical I'd go with contacting the Obs team right away :)
But also, take a look at this as suggested by one commenter in that issue

BarcoMasile
BarcoMasile previously approved these changes Sep 9, 2024
@nsklikas
Copy link
Contributor

nsklikas commented Sep 9, 2024

Thank you Marco and Nikos for your reviews. I made changes based on your feedback.
The only thing that's left and that's actually important is that alert rules aren't propagated to Loki and Grafana when using LogForwarder. The problem is in the logging library, see issue.
Nikos suggested to contact Observability team. But what would be your advice for this PR?

@pik4ez-canonical I'd go with contacting the Obs team right away :) But also, take a look at this as suggested by one commenter in that issue

The problem is that the library does not call that function, unfortunately the only thing we can do is something like this:

diff --git a/src/charm.py b/src/charm.py
index 8603562..f74aa04 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -43,6 +43,7 @@ from ops import (
     HookEvent,
     LeaderElectedEvent,
     PebbleReadyEvent,
+    RelationEvent,
     StartEvent,
     StopEvent,
     UpdateStatusEvent,
@@ -108,6 +109,11 @@ class OpenFGAOperatorCharm(CharmBase):
 
         # Loki logging relation
         self._log_forwarder = LogForwarder(self, relation_name=LOG_PROXY_RELATION_NAME)
+        # Workaround for https://github.com/canonical/loki-k8s-operator/issues/403
+        # TODO: Remove when the issue is fixed
+        self.framework.observe(self.on[LOG_PROXY_RELATION_NAME].relation_created, self._push_loki_alert_rules)
+        self.framework.observe(self.on[LOG_PROXY_RELATION_NAME].relation_changed, self._push_loki_alert_rules)
+        self.framework.observe(self.on[LOG_PROXY_RELATION_NAME].relation_departed, self._push_loki_alert_rules)
 
         # Prometheus metrics endpoint relation
         self.metrics_endpoint = MetricsEndpointProvider(
@@ -215,6 +221,9 @@ class OpenFGAOperatorCharm(CharmBase):
             "database_name": DATABASE_NAME,
         }
 
+    def _push_loki_alert_rules(self, event: RelationEvent) -> None:
+        self._log_forwarder._handle_alert_rules(event.relation)
+
     @property
     def _log_level(self) -> str:
         return self.config["log-level"]

But we will have to be mindful, so that when (if) the issue is fixed, we remove this.

Copy link
Contributor

@nsklikas nsklikas left a comment

Choose a reason for hiding this comment

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

Great job

@pik4ez-canonical pik4ez-canonical merged commit b455ec2 into main Sep 9, 2024
3 checks passed
@pik4ez-canonical pik4ez-canonical deleted the IAM-1029 branch September 9, 2024 14:42
@sed-i
Copy link

sed-i commented Sep 13, 2024

Hi team,
Thanks for the reference, fixed in loki too, and available in LIBPATCH=13.

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.

Solve the potential log file overflow issue
4 participants