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

feat(logfwd): support forwarding logs to Loki #267

Merged
merged 39 commits into from
Sep 25, 2023

Conversation

barrettj12
Copy link
Contributor

@barrettj12 barrettj12 commented Aug 1, 2023

Add a loki.Client type that encodes sends Pebble logs to Loki. This implements the logstate.logClient interface introduced in #256.

There have also been some changes to the logClient interface. Semantically, clients shouldn't flush inside the Write method - the flush should be up to the gatherer. Rename Write to AddLog for clarity, and add a NumBuffered method to help the gatherer determine when to flush.

QA steps

Deploy a Loki server for testing - this can be done with Juju. For convenience, deploy Grafana too, to view the logs.

juju bootstrap microk8s c
juju add-model cos
juju deploy loki-k8s --trust
juju deploy grafana-k8s --trust
juju integrate loki-k8s grafana-k8s:grafana-source

Wait for Loki and Grafana to reach a stable state.

Create a simple logging service:

#!/bin/bash
while true; do
  echo "hello"
  sleep 1
done

and a simple plan using this service:

services:
  svc1: &logsvc
    command: /path/to/logsvc
    startup: enabled
    override: merge
  svc2: *logsvc

log-targets:
  loki:
    type: loki
    location: http://[loki_ip]:3100/loki/api/v1/push
    override: merge
    services: [all]

[loki_ip] is the unit/app IP address from the deployed Loki in juju status.

Run Pebble:

go run ./cmd/pebble run

Get the admin password for Grafana using

juju run grafana-k8s/0 get-admin-password

and then visit [grafana_ip]:3000 in a browser, and log in with username admin and the obtained password.

If you go to the "Explore" tab and change the data source to Loki, you should be able to see logs forwarded from Pebble.


JUJU-4205

@barrettj12 barrettj12 force-pushed the loki-client branch 3 times, most recently from e4ce26c to 8f820a6 Compare August 2, 2023 04:45
@barrettj12 barrettj12 marked this pull request as ready for review August 3, 2023 03:50
@jnsgruk jnsgruk added the Priority Look at me first label Aug 3, 2023
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Nice work!

internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
Copy link
Contributor

@flotter flotter left a comment

Choose a reason for hiding this comment

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

Mostly questions for my own understanding, and perhaps something you can find useful.

internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki.go Outdated Show resolved Hide resolved
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looking good! Left a whole bunch of comments -- they're all minor though.

internals/overlord/logstate/clienterr/doc.go Outdated Show resolved Hide resolved
internals/overlord/logstate/clienterr/err.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki_test.go Outdated Show resolved Hide resolved
@barrettj12 barrettj12 changed the title [JUJU-4205] Loki client Loki client Aug 16, 2023
@barrettj12 barrettj12 changed the title Loki client feat(logfwd): Loki client Aug 16, 2023
Copy link
Contributor

@tlm tlm left a comment

Choose a reason for hiding this comment

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

lgtm @barrettj12, just some comments for your consideration.

internals/overlord/logstate/clienterr/clienterr.go Outdated Show resolved Hide resolved
internals/overlord/logstate/clienterr/clienterr.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/puller.go Outdated Show resolved Hide resolved
internals/overlord/logstate/puller.go Outdated Show resolved Hide resolved
@barrettj12 barrettj12 changed the title feat(logfwd): Loki client feat(logfwd): support forwarding logs to Loki Aug 17, 2023
@flotter flotter mentioned this pull request Aug 23, 2023
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Gentlemen, it looks like some of the lines have been blurred with this PR. I was optimistic that we'd have a nice and sweet PR since this was just implementing the Loki client, but instead this went back into the behavior that we have agreed to over the past few weeks and changed it, which unfortunately makes me ask questions about things that we had agreements on instead of it being a happy walk.

Would it be possible to do a bit less in this PR, and implement a simpler Loki client using the APIs that we had already settled on? I realize that's probably not great news from your end, so maybe have a look at these comments with a cup of tee and let me know what I got wrong?

.github/.golangci.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
internals/overlord/logstate/clienterr/clienterr.go Outdated Show resolved Hide resolved
internals/overlord/logstate/clienterr/clienterr.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/export_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
internals/overlord/logstate/clienterr/clienterr.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer.go Outdated Show resolved Hide resolved
- introduce loki.ClientArgs
- remove loki testing patch functions
@barrettj12
Copy link
Contributor Author

Just did a test pushing 2000 logs/sec at Pebble, with a fake Loki server always returning 429. It used about 10% of my laptop's CPU dealing with this, but the memory seemed to stay bounded at 0.1%. So I don't think we have an issue here with memory leaking.

Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

A couple of very nitty comments, but looks good to me!

internals/overlord/logstate/gatherer_test.go Outdated Show resolved Hide resolved
internals/overlord/logstate/gatherer_test.go Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki_test.go Outdated Show resolved Hide resolved
Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

Feel like we are finally at the end of this long road. LGTM.

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Almost there, Jordan. Thanks for your persistence.

internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
- ClientArgs -> options *ClientOptions
- rename N -> n
- explicitly reallocate buffer when full
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks, final points on that same issue, and we should be good to go:

internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
internals/overlord/logstate/loki/loki.go Outdated Show resolved Hide resolved
- inline checkBuffer
- coerce to string w/o check
Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thank you! There's still a minor related to the on going conversations, but per note below. Let's please address it in a future PR.

// resetBuffer drops all buffered logs (in the case of a successful send, or an
// unrecoverable error).
func (c *Client) resetBuffer() {
c.entries = c.entries[:0]
Copy link
Contributor

Choose a reason for hiding this comment

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

We have the same issue of entries not being released here.

I'll go ahead and merge this as we've been here for a bit :), but let's please fix this at the next opportunity.

@niemeyer niemeyer merged commit b4666dd into canonical:master Sep 25, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority Look at me first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants