lambda-promtail: Add ability to ingest logs from S3 #5065

AndreZiviani · 2022-01-06T17:11:44Z

What this PR does / why we need it:
Allows lambda-promtail to ingest load balancer logs stored on S3, before it was only possible to ingest logs from CloudWatch

Which issue(s) this PR fixes:

Special notes for your reviewer:
I've made it somewhat modular allowing to plug new datasources (e.g. SNS, SQS...) on the same lambda function easier but it is not trivial to have the same lambda receive events from multiple sources in go (it's easy on other languages) that's why I changed the event parameter to a map of interface.
Since Loki now accepts out of order writes I'm sending the timestamp from the log itself.
I've also copied the batch feature from promtail itself because a S3 log can be quite big.
Most of the changes are either inspired or straight copied from promtail source code.

Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

AndreZiviani · 2022-01-06T17:14:21Z

am I supposed to manually edit the changelog? the PR template is a little ambiguous to me

zswanson · 2022-01-06T20:30:49Z

I can't speak to the cloudformation (not my jam) but we should add the variables (bucket name, prefixes) and lambda permissions and s3 event notification to the terraform to support this.

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_notification#add-notification-configuration-to-lambda-function

trevorwhitney · 2022-01-06T22:47:03Z

@AndreZiviani yes, please manually add a changelog entry to this PR.

tools/lambda-promtail/lambda-promtail/s3.go

tools/lambda-promtail/main.tf

cstyan

This is looking good. I don't know as much about s3 and it's metadata so I will double check docs there before reading the s3 specific go code here.

tools/lambda-promtail/lambda-promtail/cw.go

tools/lambda-promtail/lambda-promtail/main.go

tools/lambda-promtail/lambda-promtail/promtail.go

AndreZiviani · 2022-01-13T13:00:54Z

sorry for those force pushes, I made a mistake when merging/rebasing...

tools/lambda-promtail/lambda-promtail/s3.go

cstyan · 2022-01-21T18:38:54Z

@AndreZiviani let me know if you need help with any of the outstanding comments :)

AndreZiviani · 2022-01-21T18:41:19Z

thanks @cstyan, I had to focus on other things this past week, will try to address your suggestions on Monday/Tuesday

cstyan

LGTM

Test failures don't look related, just a failed git clone for a sub job.

owen-d

Hey, this is looking good! Would you mind updating the docs with this new functionality?

tools/lambda-promtail/main.tf

arvindkonar

great! this is what i already did to make mine work!

… function

tools/lambda-promtail/go.sum

KMiller-Grafana

Docs portion of the PR looks good to me.

owen-d

Hey, this is looking good. Thanks for the PR! I've left a few suggestions here that could improve the approach, but I also think we could do those in a followup PR if you prefer (either done by yourself or someone else, maybe @cstyan?).

tools/lambda-promtail/lambda-promtail/s3.go

tools/lambda-promtail/lambda-promtail/promtail.go

cstyan

Looking good, a few more comments but they're mostly style nits.

I think it would be a good idea to eventually move the global variables we have in main.go that we're now using within other files for batchSize to a struct of config options or something like that, but that doesn't need to happen in this PR.

tools/lambda-promtail/lambda-promtail/main.go

cstyan · 2022-02-04T05:28:57Z

tools/lambda-promtail/lambda-promtail/promtail.go

 	if stream, ok := b.streams[labels]; ok {
 		stream.Entries = append(stream.Entries, e.entry)
-		return
+
+		b.size += stream.Size()
+
+	} else {
+
+		b.streams[labels] = &logproto.Stream{
+			Labels:  labels,
+			Entries: []logproto.Entry{e.entry},
+		}
+
+		b.size += b.streams[labels].Size()


nit again: we could simplify to something like this

stream, ok := b.streams[labels]; if !ok { b.streams[labels] = &logproto.Stream{ Labels: labels, } stream = b.stream[labels] } stream.Entries = append(stream.Entries, e.entry) b.size += stream.Size()

tools/lambda-promtail/lambda-promtail/promtail.go

cstyan · 2022-02-04T05:30:16Z

tools/lambda-promtail/lambda-promtail/s3.go

-		return nil, err
+	var s3Client *s3.Client
+
+	if c, ok := s3Clients[labels["bucket_region"]]; ok {


do we only need a single client per bucket region? there isn't the possibility of different buckets in the same region requiring different clients for auth reasons?

I suspect we can ignore this for now as multiple regions/bucket auth can be attached to this lambda.

I think multiple buckets with different auth options would increase the complexity, it would be easier to configure another lambda with another role

owen-d · 2022-02-04T14:37:47Z

tools/lambda-promtail/lambda-promtail/cw.go

@@ -38,7 +38,7 @@ func parseCWEvent(ctx context.Context, b *batch, ev *events.CloudwatchLogsEvent)
 }

 func processCWEvent(ctx context.Context, ev *events.CloudwatchLogsEvent) error {
-	batch := newBatch()
+	batch, _ := newBatch(ctx)


We should not swallow this error. Perhaps newBatch shouldn't accept entries and then newBatch won't need to return an error.

since the batch will always be empty here it should never fail, thats why I ignored it

owen-d · 2022-02-04T14:42:39Z

tools/lambda-promtail/lambda-promtail/promtail.go

+	if stream, ok := b.streams[labels]; ok {
+		stream.Entries = append(stream.Entries, e.entry)
+
+		b.size += stream.Size()


This Size() method doesn't actually measure what we're expecting (it's part of the generated protobuf). Instead, I'd keep track of the line's bytel length as they're added, like so:
https://github.com/grafana/loki/blob/main/clients/pkg/promtail/client/batch.go#L44-L45

I see, will fix that

tools/lambda-promtail/lambda-promtail/promtail.go

owen-d · 2022-02-04T14:55:56Z

tools/lambda-promtail/lambda-promtail/s3.go

-		return nil, err
+	var s3Client *s3.Client
+
+	if c, ok := s3Clients[labels["bucket_region"]]; ok {


I suspect we can ignore this for now as multiple regions/bucket auth can be attached to this lambda.

docs/sources/clients/lambda-promtail/_index.md

owen-d

Thanks, let's get this merged :)

AndreZiviani requested a review from a team as a code owner January 6, 2022 17:11

pull-request-size bot added the size/L label Jan 6, 2022

pull-request-size bot added size/XL and removed size/L labels Jan 7, 2022

james-callahan suggested changes Jan 7, 2022

View reviewed changes

tools/lambda-promtail/lambda-promtail/s3.go Outdated Show resolved Hide resolved

tools/lambda-promtail/main.tf Outdated Show resolved Hide resolved

tools/lambda-promtail/main.tf Outdated Show resolved Hide resolved

cstyan reviewed Jan 10, 2022

View reviewed changes

AndreZiviani force-pushed the feat/lambda-promtail-s3 branch from c8cd46f to 9c9f4a9 Compare January 13, 2022 12:35

AndreZiviani requested a review from KMiller-Grafana as a code owner January 13, 2022 12:35

pull-request-size bot added size/XXL and removed size/XL labels Jan 13, 2022

AndreZiviani force-pushed the feat/lambda-promtail-s3 branch from 9c9f4a9 to f560050 Compare January 13, 2022 12:42

pull-request-size bot added size/XL and removed size/XXL labels Jan 13, 2022

AndreZiviani added 9 commits January 13, 2022 09:53

Add ability to ingest logs from S3 on lambda-promtail

fd80dd0

fix ci

a2fec89

fix typo

774c5e8

bump golang and alpine version

611d604

update changelog

dc1b849

add s3 permissions on terraform

b42243e

use for_each instead of count

0ab4647

fix typo

394b6c7

improve function naming

74f88ac

AndreZiviani force-pushed the feat/lambda-promtail-s3 branch from f167211 to 74f88ac Compare January 13, 2022 12:59

cstyan reviewed Jan 14, 2022

View reviewed changes

tools/lambda-promtail/lambda-promtail/s3.go Show resolved Hide resolved

fix typo

e57b6f6

cstyan approved these changes Jan 25, 2022

View reviewed changes

owen-d reviewed Jan 25, 2022

View reviewed changes

tools/lambda-promtail/main.tf Outdated Show resolved Hide resolved

arvindkonar reviewed Jan 25, 2022

View reviewed changes

AndreZiviani added 6 commits January 26, 2022 09:30

allow lambda to access only specified s3 buckets

bdfda42

configure a default log retention policy on log group

5e75cea

add missing depends_on to make sure iam role is created before lambda…

df9eceb

… function

update docs

5cfd06a

fix label naming convention

6e5d6a3

fix merge conflicts

3b025ff

cstyan reviewed Jan 31, 2022

View reviewed changes

tools/lambda-promtail/go.sum Outdated Show resolved Hide resolved

AndreZiviani added 2 commits January 31, 2022 17:30

fix merge conflict

227f77c

use new backoff lib and update dependencies

b99d800

KMiller-Grafana approved these changes Feb 1, 2022

View reviewed changes

owen-d approved these changes Feb 1, 2022

View reviewed changes

tools/lambda-promtail/lambda-promtail/s3.go Outdated Show resolved Hide resolved

tools/lambda-promtail/lambda-promtail/promtail.go Outdated Show resolved Hide resolved

AndreZiviani added 3 commits February 3, 2022 12:24

add option to limit batch size

2c241b7

cache s3 client

b443ebc

update docs and terraform

7d3c4f8

AndreZiviani requested a review from owen-d February 3, 2022 15:50

cstyan reviewed Feb 4, 2022

View reviewed changes

owen-d reviewed Feb 4, 2022

View reviewed changes

address some feedback on PR

370f116

AndreZiviani requested review from cstyan and owen-d February 4, 2022 19:07

fix typo

b31063a

owen-d approved these changes Feb 4, 2022

View reviewed changes

owen-d merged commit 699fffe into grafana:main Feb 4, 2022

slim-bean mentioned this pull request Mar 2, 2022

lambda-promtail no longer working with basic auth #5518

Closed

This was referenced Mar 18, 2022

Design doc: object store scrape #2107

Closed

Feature: Promtail, scrape logs from Object store #2270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lambda-promtail: Add ability to ingest logs from S3 #5065

lambda-promtail: Add ability to ingest logs from S3 #5065

AndreZiviani commented Jan 6, 2022 •

edited

Loading

AndreZiviani commented Jan 6, 2022

zswanson commented Jan 6, 2022

trevorwhitney commented Jan 6, 2022

cstyan left a comment

AndreZiviani commented Jan 13, 2022

cstyan commented Jan 21, 2022

AndreZiviani commented Jan 21, 2022

cstyan left a comment

owen-d left a comment

arvindkonar left a comment

KMiller-Grafana left a comment

owen-d left a comment

cstyan left a comment

cstyan Feb 4, 2022

cstyan Feb 4, 2022

owen-d Feb 4, 2022

AndreZiviani Feb 4, 2022

owen-d Feb 4, 2022

AndreZiviani Feb 4, 2022

owen-d Feb 4, 2022

AndreZiviani Feb 4, 2022

owen-d Feb 4, 2022

owen-d left a comment

lambda-promtail: Add ability to ingest logs from S3 #5065

lambda-promtail: Add ability to ingest logs from S3 #5065

Conversation

AndreZiviani commented Jan 6, 2022 • edited Loading

AndreZiviani commented Jan 6, 2022

zswanson commented Jan 6, 2022

trevorwhitney commented Jan 6, 2022

cstyan left a comment

Choose a reason for hiding this comment

AndreZiviani commented Jan 13, 2022

cstyan commented Jan 21, 2022

AndreZiviani commented Jan 21, 2022

cstyan left a comment

Choose a reason for hiding this comment

owen-d left a comment

Choose a reason for hiding this comment

arvindkonar left a comment

Choose a reason for hiding this comment

KMiller-Grafana left a comment

Choose a reason for hiding this comment

owen-d left a comment

Choose a reason for hiding this comment

cstyan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owen-d left a comment

Choose a reason for hiding this comment

AndreZiviani commented Jan 6, 2022 •

edited

Loading