lambda-promtail: s3: update log passing to support CLB & NLBs

[elliotdobson]

What this PR does / why we need it: Fixes the filename and timestamp regex for AWS CLB & NLB logs. Also adds some logic to make NLB log timestamps RFC3339 compatible

Which issue(s) this PR fixes: Fixes #6455

Special notes for your reviewer:

Checklist

• [ ] Documentation added
• [ ] Tests updated
• [ ] Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
• [ ] Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

[CLAassistant]

All committers have signed the CLA.

[grafanabot]

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

[MichelHollands]

@elliotdobson Hello, thanks for your contribution. Can you add some unit tests to check the regex against the filenames of the various types of load balancer? Also, there is a merge conflict so this should be rebased. Once these are done I can merge it.

[jeschkies]

@elliotdobson would you still be up to Adresse the requested changes? Or shall we take over?

[elliotdobson]

@jeschkies sure. Do you have any docs on running the tests for lambda-promtail?

[MasslessParticle]

@elliotdobson just run go test ./... from the package where you write the test

[jeschkies]

@elliotdobson Just a unit test would suffice.

[elliotdobson]

Hey all, I've rebased onto latest upstream main and found that others had already added some tests for the S3 code so I've extended this for NLB.

Unfortunately AWS didn't make it easy and the log format is different for each load balancer. For now I've removed CLB support and am focusing on NLB.

I'd like to add a test for the parsed timestamp given that ALB & NLB have different timestamp formats but I am not sure how to get this field in Test_parseS3Log. Any advice?

[cstyan]

@elliotdobson another PR has been merged so you may want to rebase again.

It looks to me like we have a testdata folder with example logs people have saved, and they're then loading those into tests to ensure things are parsed and processed correctly. As far as testing via the s3 event or the other event type, I think you want a new test case that tests your load balancer logs from a file? If I understand correctly these are S3 events for load balancer logs and not a different event type themselves?

[elliotdobson]

Thanks @cstyan, I've added an example log file for NLB into the testdata folder and added a test to Test_parseS3Log here.

However it does not look like Test_parseS3Log is actually comparing the logs that are parsed from testdata folder are as expected. Test_parseS3Log looks at some expected label values that we're passing in with the test.

For example here is the labels we're passing into the test, and here is what we're expecting back. If I understand correctly this does not test what is actually parsed from the log, only that the log file can be opened.

In the parseS3Log function we try to match the timestamp in the log line and if we get a match we set that as the timestamp for the logproto.Entry. So I think we need to add a test that confirms the timestamp regex is working as expected.

However I am not sure how to access a logproto.Entry during the test. Do you know?

[cstyan]

You're right, the test doesn't actually account for proper parsing of the timestamps. It's a bit confusing, but the labels in args are the actual event labels. Since lambda-promtail can receive various AWS event types, that's what the test is currently checking for. Another test for the timestamp parsing specifically is probably a good idea.

One of the arguments to parseS3log is a pointer batch struct, the batch itself will contain the stream(s) that will be sent to Loki: https://github.com/elliotdobson/loki/blob/lambda-promtail-aws-nlb-logs/tools/lambda-promtail/lambda-promtail/promtail.go#L39-L43

The stream has a field for entries of type logproto.Entries, you can see how we add them here: https://github.com/elliotdobson/loki/blob/lambda-promtail-aws-nlb-logs/tools/lambda-promtail/lambda-promtail/promtail.go#L67-L86

So my suggestion would be it parse all the log lines from your example file into a single stream, get that stream from the map in the batch, and iterate over the entries for that stream while checking the timestamps. We should compare the timestamps to an expected value, not just that any timestamp was parsed from each line in the file.

[cstyan]

@elliotdobson please ping me when you need another review

[elliotdobson]

Hi @cstyan, I have just rebased on main. It is ready for another review please.

[cstyan]

I think this is probably good to go. Do you mind cherry picking this commit into your branch? I noticed we're not running the lambda-promtail tests on PR's/in CI so we don't know if a new PR is breaking something.

[elliotdobson]

@cstyan done. FWIW I did run go test ./... locally and it passed.

[cstyan]

@elliotdobson awesome! yeah previously even locally, from the root of the repo, go test ./... wouldn't have run the lambda-promtail tests. It will now though