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

Lambda-promtail: Enhance lambda-promtail to support adding extra labels from an environment variable value #5359

Merged
merged 22 commits into from
Feb 16, 2022

Conversation

JBSchami
Copy link
Contributor

@JBSchami JBSchami commented Feb 9, 2022

What this PR does / why we need it:
This PR gives users of lambda-promtail the ability to add extra labels to logs forwarded from both S3 and Cloudwatch sources.

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

Special notes for your reviewer:

Checklist

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

@JBSchami JBSchami requested a review from a team as a code owner February 9, 2022 21:41
@CLAassistant
Copy link

CLAassistant commented Feb 9, 2022

CLA assistant check
All committers have signed the CLA.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 9, 2022
Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

I think it would make more sense to just load the env var as a comma separated list of label name and value, such as name1,value1,name2,value2. Then just split on each , and if the len is not divisible by 2 then we panic with a message that the env var is invalid.

…in the form of "name1,value1,name2,value2,..."
)

func init() {
func configure() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not super familiar with Go, but having this as init() was making it impossible to run the tests. I renamed to configure() and call it from main() let me know if that is a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets name this something a bit more descriptive like setupArguments. Here's a description of what the init function is: https://go.dev/doc/effective_go#init

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

func init() {
func configure() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets name this something a bit more descriptive like setupArguments. Here's a description of what the init function is: https://go.dev/doc/effective_go#init

tools/lambda-promtail/lambda-promtail/main_test.go Outdated Show resolved Hide resolved
tools/lambda-promtail/lambda-promtail/main_test.go Outdated Show resolved Hide resolved
…y extra labels code to utility method, replaced hardcoded string with const global var in module.
@JBSchami
Copy link
Contributor Author

The continuous-integration/drone/pr failed while trying to clone mixins is there any way to re-run the check (without committing anything)?

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 11, 2022
Copy link
Contributor

@cstyan cstyan 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, one more comment that should be addressed IMO

tools/lambda-promtail/lambda-promtail/main.go Outdated Show resolved Hide resolved
@JBSchami JBSchami requested a review from cstyan February 14, 2022 21:21
@JBSchami
Copy link
Contributor Author

@cstyan any way for me to retry the CI without committing anything? It failed on clone for Docker-amd64 which doesn't really have anything to do with this PR.

@cstyan
Copy link
Contributor

cstyan commented Feb 16, 2022

@JBSchami reran the tests and it looks like everything's good.

One last thing, could you add a short example to docs/sources/clients/lambda-promtail/_index.md. I'll let the rest of the team know that this is good to merge once we have an example in the docs.

@owen-d owen-d merged commit 776d5c4 into grafana:main Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lambda-Promtail: add ability to add custom tags to all logs forwarded from Cloudwatch
5 participants