Title says it all. This should probably be done at the same time as we merge the two libraries.

Bug Description

The following method:

    def _endpoints(self) -> List[dict]:
        """Return a list of Loki Push Api endpoints."""
        return [{"url": self._url(unit_number=i)} for i in range(self._charm.app.planned_units())]

in LokiPushApiProvider class relies on planned units numbers to generate Loki endpoints.

But lifecycle reasons mean that at any given point unit numbers may not start with 0, may also have gaps, for instance if unit 5 is faulty I would remove it and add a new one, I'd end up with units 4, 6, 7.

To Reproduce

There is no need to have steps to reproduce this.

Environment

• juju 2.9.25

Relevant log output

None

Additional context

No response

Bug Description

When a charm author includes the loki_push_api library, if they do not create the src/loki_alert_rules folder, the library spits WARNING logs continuously. We should issue the WARNING logs if and only if: the charm author has CUSTOMIZED the alert rules path AND the directory does not exist. By default, we should support charm authors not to set alert rules and not to be spammed :-)

To Reproduce

• Add the LokiPushApiConsumer to a charm, do not create the src/loki_alert_rules folder
• charmcraft pack
• juju deploy ...
• juju debug-log
• enjoy the spam

Environment

Wherever

Relevant log output

THere is too much log

Additional context

No response

Bug Description

Loki charmed in standalone mode must implement an ingress relation using the ingress_per_unit relation interface and the https://github.com/canonical/traefik-k8s-operator/blob/main/lib/charms/traefik_k8s/v0/ingress_per_unit.py library. The URL received by Loki must be used to produce the URL of the Loki Push API endpoint, and the

Note: The path of the LokiPushApi endpoint will be affected by an ingress using a path routing rule, see e.g. https://github.com/canonical/traefik-k8s-operator#configurations

Note: The future charming of Loki using the distributed mode, will instead use the https://github.com/canonical/traefik-k8s-operator/blob/main/lib/charms/traefik_k8s/v0/ingress.py library and the respective ingress relation interface, as the Loki ingester components are designed to be load-balanced, rather than exposed as separate endpoints.

To Reproduce

None

Environment

• juju
• microk8s

Relevant log output

None

Additional context

No response

Bug Description

The promtail binary currently used in the lib is dynamically linked. This means it won't work on all Linux environments. We need to build our own version from upstream that is statically linked (ie. with CGO disabled). The suggested course of action, for now, is to build it ourselves, place it in the Loki repo, and fetch it from there instead of directly from the upstream repo.

To Reproduce

1. wget https://github.com/grafana/loki/releases/download/v2.4.1/promtail-linux-amd64.zip
2. unzip promtail-linux-amd64.zip
3. file promtail-linux-amd64

Environment

N/A

Relevant log output

❯ file promtail-linux-amd64

promtail-linux-amd64: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0, Go BuildID=ZZTDSIbZy_VfnPWWwZOz/C9YTBAHGKginqRccK1ae/XVCFXE7cTRaMg17D9-Xy/drsoguGJyTXhi9FV8Vbu, BuildID[sha1]=c4e8062fc97a9e5fd59355a162aba1bab0609cac, stripped

Additional context

No response

Loki config missing in grafana-agent charm.

how can it be that with the latest Grafana Agent edge, when I relate Grafana Agent with Loki, I get no loki configurations generated?

michele@boombox:~$ juju status --relations
Model  Controller   Cloud/Region        Version  SLA          Timestamp
lma    development  microk8s/localhost  2.9.22   unsupported  18:56:09+01:00

App            Version  Status  Scale  Charm              Store     Channel  Rev  OS          Address         Message
alertmanager            active      1  alertmanager-k8s   charmhub  edge       7  kubernetes  10.152.183.187  
grafana                 active      1  grafana-k8s        charmhub  edge      14  kubernetes  10.152.183.218  
grafana-agent           active      2  grafana-agent-k8s  charmhub  edge       4  kubernetes  10.152.183.232  
loki                    active      1  loki-k8s           charmhub  edge      11  kubernetes  10.152.183.168  
prometheus              active      1  prometheus-k8s     charmhub  edge      13  kubernetes  10.152.183.139  

Unit              Workload  Agent  Address      Ports  Message
alertmanager/0*   active    idle   10.1.151.76         
grafana-agent/0*  active    idle   10.1.151.80         
grafana-agent/1   active    idle   10.1.151.81         
grafana/0*        active    idle   10.1.151.77         
loki/0*           active    idle   10.1.151.78         
prometheus/0*     active    idle   10.1.151.79         

Relation provider          Requirer                 Interface              Type     Message
alertmanager:alerting      prometheus:alertmanager  alertmanager_dispatch  regular  
alertmanager:replicas      alertmanager:replicas    alertmanager_replica   peer     
grafana:grafana-peers      grafana:grafana-peers    grafana_peers          peer     
loki:grafana-source        grafana:grafana-source   grafana_datasource     regular  
loki:logging               grafana-agent:logging    loki_push_api          regular  
prometheus:grafana-source  grafana:grafana-source   grafana_datasource     regular 

michele@boombox:~$ microk8s.kubectl exec -it grafana-agent-0 -n lma -c agent -- cat /etc/agent/agent.yaml
integrations:
  agent:
    enabled: true
    relabel_configs:
    - regex: (.*)
      replacement: lma_f149bca2-4d3e-4289-8b72-6b66b40d14dc_grafana-agent_grafana-agent/0
      target_label: instance
    - replacement: grafana-agent-k8s
      source_labels:
      - __address__
      target_label: juju_charm
    - replacement: lma
      source_labels:
      - __address__
      target_label: juju_model
    - replacement: f149bca2-4d3e-4289-8b72-6b66b40d14dc
      source_labels:
      - __address__
      target_label: juju_model_uuid
    - replacement: grafana-agent
      source_labels:
      - __address__
      target_label: juju_application
    - replacement: grafana-agent/0
      source_labels:
      - __address__
      target_label: juju_unit
  prometheus_remote_write: []
loki: {}
prometheus:
  configs:
  - name: agent_scraper
    remote_write: []
    scrape_configs: []
server:
  log_level: info

Issue

LokiPushApiProvider is deleting the entire alert rules directory every time a charm departs the relation. So if we have 2 charms related to Loki and one of them departs the relation, the _remove_alert_rules_files deletes everything.

Expected Behavior

We need to delete just the files of the charm that departs the relation.

Additional info, logs, etc:

    def _remove_alert_rules_files(self, container: Container) -> None:
        """Remove alert rules files from workload container.

        Args:
            container: Container which has alert rules files to be deleted
        """
        container.remove_path(self._rules_dir, recursive=True)
        logger.debug("Previous Alert rules files deleted")
        # Since container.remove_path deletes the directory itself with its files
        # we should create it again.
        os.makedirs(self._rules_dir, exist_ok=True)

Possible Fix

In _remove_alert_rules_files we should retrieve alert rules from the file system and delete the files of the charm that departs the relation.

Steps to Reproduce

Test to verify this works as expected:

1. Deploy Spring Music and Grafana Agent in a separate model
2. Deploy another Spring Music in the same model as Loki
3. Relate the Spring Music instance in the same model as Loki directly to Loki
4. Relate Spring Music to Grafana Agent
5. Relate Grafana Agent to Loki over a CMR
6. Remove the relation
7. Make sure the correct files are deleted

Do the same with a direct relation between Spring Music and Loki. Experiment with different variations.

Context

Your Environment

• Charm Version used: main branch
• Environment name and version (e.g. Juju, Microk8s, etc):
  • Juju: 2.9.22
  • microk8s: 1.23/stable
• Operating System and version (e.g Ubuntu 20.04): Ubuntu 20.04

loki_push_api_alert_rules_error is well scoped, and is an instance of the well named LokiPushApiAlertRulesError.
I wonder if we should drop the prefix here

    alert_rules_error = EventSource(LokiPushApiAlertRulesError)

so the user could just

        self.framework.observe(
            self._loki_consumer.on.alert_rules_error,
            self._alert_rules_error
        )

Originally posted by @sed-i in #45 (comment)

If we remove the prefix here, we should also remove:

class LoggingEvents(ObjectEvents):
    """Event descriptor for events raised by `LokiPushApiProvider`."""

    loki_push_api_alert_rules_error = EventSource(LokiPushApiAlertRulesError)
    loki_push_api_endpoint_departed = EventSource(LokiPushApiEndpointDeparted)
    loki_push_api_endpoint_joined = EventSource(LokiPushApiEndpointJoined)

Will create an issue to address it later.

Also, not sure what it means that no peer relation is defined in metadata.

It means nothing special. We could use a peer relation to observe wether the Loki charm is scaled to more than one unit, and > put it in blocked state like we do it with Prometheus + Ingress thou.

Originally posted by @mmanciop in #48 (comment)

It is my understanding that the container_name in [container_name]-pebble-ready are a dash-to-underscore mapped version of the 'actual' container name.
LogProxyConsumer fails to implement this same logic and therefore gives:

  File "./src/charm.py", line 29, in __init__                                                                          
    self.log_proxy_consumer = LogProxyConsumer(                                                                        
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/lib/charms/loki_k8s/v0/loki_push_api.py", line 1624, in __init__ 
    getattr(self._charm.on, "{}_pebble_ready".format(self._container_name)),                                           
AttributeError: 'CharmEvents' object has no attribute 'loki-tester_pebble_ready'                                       

When the container name is 'loki-tester'. Note that that is not a valid python identifier.

Solve by, in __init__:

        self.framework.observe(
            getattr(self._charm.on, "{}_pebble_ready".format(self._container_name.replace("-", "_"))),
            self._on_pebble_ready,
        )

#2 (comment)

Since charmcraft is very strict with the LIBPATCH correlation, downgrade this value to: 6

$ charmcraft publish-lib charms.loki_k8s.v0.loki_push_api
Library charms.loki_k8s.v0.loki_push_api has a wrong LIBPATCH number, it's too high, Charmhub highest version is 0.5.

Seems like there is an implicit assumption that event is either rel-changed or rel-dep.
Maybe in this case two different hooks would be better?

Originally posted by @sed-i in #33 (comment)

This property now may return:

• A list
• A string
• None

Maybe a better contract for consumes will be a property that always returns a list:

• A list with many elements
• A list with one element
• An empty list

Originally posted by @Abuelodelanada in #48 (comment)

_is_official_alert_rule_format(rule_file)
raises an uncaught TypeError: argument of type 'NoneType' is not iterable
when loki_alert_rules is an empty file.

Is that a valid 'rules' file to begin with?
Either way, looks like we should return list() in _from_file even if the file is invalid/empty.

Per the naming guidelines recently published, this repo should be renamed loki-k8s-operator. Will need a ticket into IS.

Probably not very relevant for our use case but see comment on SO for buffering large files.

Originally posted by @sed-i in #42 (comment)

Change the name to be MultipleContainersFoundError and clean up the error message

on upgrade should probably also call _configure, and possibly manipulate self._stored.config as well.

Originally posted by @sed-i in #11 (comment)

This happened. Let's make sure it doesn't happen again.

#33 (comment)

iirc when the dir is already created an exception is raised.

I would consider delegating this concern to a separate class, for example

alerts = list(map(LokiAlertRule.from_file, Path(self._alert_rules_path).glob("*.rule")))

Originally posted by @sed-i in #25 (comment)

Consider generating topology info with a dedicated function/class.

return JujuTopology(charm).as_dict()  # TypedDict

Originally posted by @sed-i in #25 (comment)

I deployed loki from charmhub edge and after a while noticed the following recurring error in the log.
IIRC this was the reason I stopped used autostart in alertmanager.

unit-loki-0: 02:13:46 ERROR unit.loki/0.juju-log Uncaught exception while in charm code:
Traceback (most recent call last):
  File "./src/charm.py", line 109, in <module>
    main(LokiOperatorCharm)
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/main.py", line 409, in main
    _emit_charm_event(charm, dispatcher.event_name)
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/main.py", line 143, in _emit_charm_event
    event_to_emit.emit(*args, **kwargs)
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/framework.py", line 278, in emit
    framework._emit(event)
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/framework.py", line 722, in _emit
    self._reemit(event_path)
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/framework.py", line 769, in _reemit
    custom_handler(event)
  File "./src/charm.py", line 79, in _on_loki_pebble_ready
    container.autostart()
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/model.py", line 1111, in autostart
    self._pebble.autostart_services()
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/pebble.py", line 822, in autostart_services
    return self._services_action('autostart', [], timeout, delay)
  File "/var/lib/juju/agents/unit-loki-0/charm/venv/ops/pebble.py", line 862, in _services_action
    raise ChangeError(change.err, change)
ops.pebble.ChangeError: cannot perform the following tasks:
- Start service "loki" (service "loki" was previously started)

What, no integration test? :-)

Originally posted by @mmanciop in #72 (comment)

See

I think the pattern we've discussed using until we come up with something better is to have the lib emit some kind of error event, and have the charm author listen to that and decide themselves whether to block or not.

Originally posted by @simskij in #67 (comment)

I am not sure why you bother to check if the current config is different from the saved one. Why not just always push the current config and simplify your code flow ?

Originally posted by @balbirthomas in #11 (comment)

If this is in past tense then should probably be under _generate?

Originally posted by @sed-i in #48 (comment)

Bug Description

Every time the constructor is called, an HTTP request is sent to Loki asking it for its version. This information is then logged into the debug logs, spamming them with unneeded log lines.

To Reproduce

1. juju deploy loki-k8s --channel edge
2. look in the debug logs

Environment

charm version: latest/edge

Relevant log output

N/A

Additional context

No response

That's kind of a can of worms.

The initial impetus behind that OF PR came about early in the charming campaign, when Balbir quickly realized that YAML/JSON dumping didn't work on Stored* objects, because they don't behave like the types they're named after (in a rather large variety of ways, but in that way particularly).

The PR was principally about hiding this from users by implementing equality methods with transparent conversions, and letting them be converted to JSON/YAML without the use of an explicit YAML/JSON formatter. My feeling then (and now) is that it doesn't feel Pythonic to have StoredDict.as_dict(), or to need a formatter for YAML/JSON, since a FooDict and FooList should just behave like the types they are claiming to be.

In the end, we had a discussion about whether StoredState (and the objects underneath it) are/were even appropriate places to keep data. You can't put the cat back in the bag, but certainly "we" at Canonical can write charms in a way which reflects best practice. For resiliency, as your recent comments about StoredState being harmful allude to, we may be better off creating peer relation buckets as the default and keeping data there, which is battle-tested, has a well-known API (anything that can be converted to/from a string), etc.

I'd suggest that, instead of going the route of explicit conversions (which will actually be _type_convert_stored anyway), a peer relation bucket is added and used here instead. The code block in the middle of this comment trivially demonstrates why. The existing magic conversions in OF already turn a list under a dict into a StoredList. Every node under the parent becomes a Stored* type, and they all need to be converted back recursively if it needs to work with json.dumps() or yaml.dump(), so an explicit conversion of StoredDict.to_dict() is necessarily more or less identical to _type_convert_stored, and the same goes for StoredList.to_list().

Let's add and use a peer relation bucket, which gives anyone reading the code for this when writing a new charm a sense of "best practice" without list of footnotes illustrating the footguns from StoredState

Originally posted by @rbarry82 in #48 (comment)

If the juju admin scales up loki without proper HA support, we need to supply sufficient warnings. Log messages and possibly status messages should explain the issues with doing it this way and make it clear that it is not supported.

Should be renamed to LokiPushApiRequirer throughout.

This is not very high priority since it should only happen when another bug causes the file to not exist. (At time of writing such a bug exists #60).

The bug here is that the _build_config_file requires the config file to exist already. This seems like it could possibly lead to more serious issues down and we should consider fixing it.

Commenting here so it's close, since I missed the last review, but as happy as I am to see a nested function as _group_name, I suppose it should be moved out since we decided not to use this as a pattern.

Sorry in advance, since this is long for a part of the library which isn't even in this PR.

def _group_name(path) -> str: should be top-level (and then typing it as typing.Union[Path.PosixPath, Path.WindowsPath]) wouldn't hurt. and using path.relative_to

Also:

return "{}_{}alerts".format(
    topology.identifier, "" if relpath == "." else relpath.replace(os.path.sep, "_") + "_"
)

This hurts to parse.

This is a nit, really, and I hate commenting on committed code, but it is a nested function, and as long as it's being moved, it could look nicer. os.path is ok, but the format is definitely a place where it's not intuitive when + "_" is actually applied, and re-reading it to grok it is slow. It looks like a smell, and the double ) from .format() and .replace() makes me worry that later patches could subtly break it.

Arguably, the current formatting is a fragile, since relative paths with "../" or something, absolute paths will lead to __, this would not work on any Windows charms, as a library function in the proxy, etc. May as well code defensively as long as I'm looking at it.

It would be very possible, right now, to get a string like the following on Windows (or just keep the .. part on Linux), and that may be an ugly bug.

{topology.identifier)_C:_foo_.._bar_baz_alerts

Pathlib ends up being somewhat more verbose, but much more flexible/resilient. Even if it isn't used, we should absolutely strip out ../ and ^([A-Z]+:)?/ with a regex and stick with the rest of os.path.

relpath = path.parent.relative_to(dir_path)

# We should account for both absolute paths and Windows paths. Convert it to a POSIX
# string, strip off any leading /, then join it

if relpath.samefile(Path(".")):
    path_str = ""
else:
    # Get rid of leaving / and optionally drive letters so they don't muck up the template later,
    # since Path.parts returns them. The 'if relpath.is_absolute ...' isn't even needed since re.sub
    # doesn't throw exceptions if it doesen't match, so it's optional, but it makes it clear what we're
    # doing.
    
    # Note that Path doesn't actually care whether the path is valid just to instantiate the object, so
    # we can happily strip that stuff out to make templating nicer
    relpath = Path(re.sub(r'^([A-Za-z]+:)?/', '', relpath.as_posix()) if relpath.is_absolute() else relpath
    
    # Get rid of relative path characters in the middle which both os.path and pathlib leave hanging
    # around. We could use path.resolve(), but that would lead to very long template strings when
    # rules come from pods and/or other deeply nested charm paths
    char_filter = lambda x: x not in ["..", "/"]
    path_str = "{}_".format("_".join(filter(char_filter, relpath.parts)))

return "{}_{}alerts.format(
    topology.identifier, path_str
)

Originally posted by @rbarry82 in #45 (comment)

We should take some time to investigate whether worth or not. Perhaps we ca use it in other charms.

#33 (comment)

If possible, I'd avoid using storage inside a library.

Originally posted by @mmanciop in #25 (comment)

Now, both libraries uses the same interface name: loki_push_api and they do different things:

• loki_push_api shares Loki Push API endpoint over relation data
• log_proxy shares Loki Push API endpoint over relation data AND Injects Promtail in workload charm

• Consider renaming file e.g. test_multiple_rule_providing_apps: - Originally posted by @sed-i in #99 (comment)
• async def deploy_rule_providing_app(app_name: str)? - Originally posted by @sed-i in #99 (comment)
• If instead you remove rules_app2 then you could easily have a stronger assertion: assert rules_after_delete_relation2 == rules_after_relation - Originally posted by @sed-i in #99 (comment)
• Add upgrade_charm test, see: #97

Do you always need to restart. Does loki allow reload ?

Originally posted by @balbirthomas in #11 (comment)

While debugging cos-config I found out that the following rule never reaches loki (doesn't end up in relation data).
This sample file is taken from the official docs:
https://grafana.com/docs/loki/latest/rules/#example

groups:
  - name: should_fire
    rules:
      - alert: HighPercentageError
        expr: |
          sum(rate({app="foo", env="production"} |= "error" [5m])) by (job)
            /
          sum(rate({app="foo", env="production"}[5m])) by (job)
            > 0.05
        for: 10m
        labels:
            severity: page
        annotations:
            summary: High request latency
  - name: credentials_leak
    rules: 
      - alert: http-credentials-leaked
        annotations: 
          message: "{{ $labels.job }} is leaking http basic auth credentials."
        expr: 'sum by (cluster, job, pod) (count_over_time({namespace="prod"} |~ "http(s?)://(\\w+):(\\w+)@" [5m]) > 0)'
        for: 10m
        labels: 
          severity: critical

Would be nice if unit status could be shorter, or not change so much that it fills the logs.

$ juju show-status-log loki/0
Time                   Type       Status       Message
09 Feb 2022 23:29:36Z  workload   waiting      installing agent
09 Feb 2022 23:29:36Z  juju-unit  allocating   
09 Feb 2022 23:30:29Z  juju-unit  allocating   Error: ImagePullBackOff
09 Feb 2022 23:30:29Z  workload   waiting      agent initializing
09 Feb 2022 23:30:41Z  workload   maintenance  installing charm software
09 Feb 2022 23:30:41Z  juju-unit  executing    running install hook
09 Feb 2022 23:30:57Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f02c46ca670>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:30:59Z  juju-unit  executing    running grafana-source-relation-created hook
09 Feb 2022 23:31:02Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f9e45532940>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:31:03Z  juju-unit  executing    running leader-elected hook
09 Feb 2022 23:31:04Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f6cfb3aa9d0>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:31:04Z  juju-unit  executing    running active-index-directory-storage-attached hook
09 Feb 2022 23:31:06Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7faf5d25fb50>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:31:07Z  juju-unit  executing    running loki-chunks-storage-attached hook
09 Feb 2022 23:31:10Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f928e5c23d0>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:31:10Z  juju-unit  executing    running config-changed hook
09 Feb 2022 23:31:12Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7ff8467d4be0>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:31:13Z  juju-unit  executing    running start hook
09 Feb 2022 23:31:15Z  juju-unit  allocating   Pulling image "registry.jujucharms.com/charm/ayt4gaga2x20gye32qthgygevco13qgq6rfff/loki-image@sha256:b61a65dbf591ccebebf3874c8d5e37d4ba1a8d2017625da97a83a4ced2ff252e"
09 Feb 2022 23:31:15Z  workload   waiting      Waiting for Pebble ready
09 Feb 2022 23:31:17Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fd9090bac10>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:31:18Z  juju-unit  executing    running grafana-source-relation-joined hook for grafana/0
09 Feb 2022 23:31:19Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fa08aa89df0>: Failed to establish a new connection: [Errno 111] Connection refused'))
09 Feb 2022 23:31:20Z  juju-unit  executing    running grafana-source-relation-changed hook for grafana/0
09 Feb 2022 23:31:23Z  juju-unit  idle         
09 Feb 2022 23:36:22Z  juju-unit  allocating   Back-off pulling image "registry.jujucharms.com/charm/ayt4gaga2x20gye32qthgygevco13qgq6rfff/loki-image@sha256:b61a65dbf591ccebebf3874c8d5e37d4ba1a8d2017625da97a83a4ced2ff252e"
09 Feb 2022 23:36:22Z  workload   waiting      HTTPConnectionPool(host='localhost', port=3100): Max retries exceeded with url: /loki/api/v1/status/buildinfo (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fb9a4849d30>: Failed to establish a new connection: [Errno 111] Connection refused'))

The LogProxyConsumer has a remarkably different API than LogPushApiConsumer, in that is does not emit events, and I really do not like this. I feel very strongly that the charm author experience should be virtually the same.

Originally posted by @mmanciop in #67 (review)

Shouldn’t LogProxyConsumer also observe relation_broken for safety? Also it needs relation_joined for new consumer units being spun up, e.g., after a pod churn, as those do not get the relation_created, and I am not sure relation_changed will fire.

LokiPushApiProvider: alert rules probably should be deleted on relation_broken: relation_departed is for single units going away, but alert rules are global to the consumer Juju app. Also, same comment about relation_joined applies.

Originally posted by @mmanciop in #63 (comment)

While working on cos-config I found out that alert rules that I forward to Loki are rejected: Loki sees them but complains about syntax

> $ curl 10.152.183.14:3100/loki/api/v1/rules                                                                                                                                                              
failed to list rule group for user fake and namespace 0_cos-configuration-k8s_alert.rules: error parsing /loki/rules/fake/0_cos-configuration-k8s_alert.rules: /loki/rules/fake/0_cos-configuration-k8s_alert.rules: could not parse expression: parse error at line 1, col 1: syntax error: unexpected IDENTIFIER

but the only thing in line 1 is groups:.

Apparently,

you need at least one equality matcher
cortexproject/cortex#3875 (comment)

When I replaced the rule with the following, Loki was happy

groups:
 - name: example
   rules:
     - alert: HighThroughputLogStreams
       expr: absent_over_time({namespace="dev",job=~".*-logs"}[2m])
       for: 2m

Seems like the loki lib needs to check for this so the user could be prompted.

This should be using the consumer base class, shouldn't it? The log proxy is currently lacking support for alerts altogether 😅

Originally posted by @simskij in #67 (comment)

The Loki unit gets put into BlockedState when one or more of its alert rules are invalid, irrespective of which relation provides them.

However, at this line we are not checking whether the alert rules are now correct before continuing with the _configure method, so the BlockedStatus can effectively be overridden by anything that will trigger a _configure call.

Promtail binaries should be uploaded to charmhub as a Loki resource.

This happens from time to time on juju remove-application loki-k8s; might be a timing issue, if the relation-departed is queued after the databag gets deleted?

in _on_relation_departed                                                                                  
    new_config = self._promtail_config                                                                    
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/lib/charms/loki_k8s/v0/loki_push_api.py", line 1890,
in _promtail_config                                                                                       
    config = {"clients": self._clients_list()}                                                            
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/lib/charms/loki_k8s/v0/loki_push_api.py", line 1904,
in _clients_list                                                                                          
    clients = clients + json.loads(relation.data[relation.app]["endpoints"])                              
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/venv/ops/model.py", line 430, in __getitem__        
    return self._data[key]                                                                                
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/venv/ops/model.py", line 414, in _data              
    data = self._lazy_data = self._load()                                                                 
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/venv/ops/model.py", line 779, in _load              
    return self._backend.relation_get(self.relation.id, self._entity.name, self._is_app)                  
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/venv/ops/model.py", line 1588, in relation_get      
    return self._run(*args, return_output=True, use_json=True)                                            
  File "/var/lib/juju/agents/unit-loki-tester-0/charm/venv/ops/model.py", line 1523, in _run              
    raise ModelError(e.stderr)                                                                            
ops.model.ModelError: b'ERROR permission denied\n'