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: parameterise the MaximumEventAgeInSeconds, LogGroupName, and IAMRoleName for lambda-promtail CloudFormation template #12728

Conversation

InsomniaCoder
Copy link
Contributor

@InsomniaCoder InsomniaCoder commented Apr 22, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:

It adds the parameter for specifying MaximumEventAgeInSeconds for the lambda-promtail's EventInvokeConfig template.

Why do we need this?

Without specifying this, the default value is 21600 which is 6 hours. We have been facing a problem where our lambda-promtail gets throttled and cannot process cloudwatch logs fast enough.

This has an effect as the more throttled it is, the more delay the event got processed, and eventually the old messages will be too old for Loki, causing even more failure to the Lambda processing

server returned HTTP status 400 Bad Request (400): entry with timestamp 2024-04-22 08:10:31.966 +0000 UTC ignored, reason: 'entry too far behind, oldest acceptable timestamp is: 2024-04-22T08:21:38Z',: errorString

image

image

Once we got to this point, the only way to mitigate this is to discard the old events, and the only way to do it is to change the MaximumEventAgeInSeconds

image

As the current template does not allow this, we are making this PR so we can benefit from upstream fix.

Test

Using this version of the lambda-promtail.yaml and put 100 seconds as MaximumEventAgeInSeconds

image

Checking the configuration of the new version, it's updated as it should

image

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@InsomniaCoder InsomniaCoder requested a review from a team as a code owner April 22, 2024 15:06
@CLAassistant
Copy link

CLAassistant commented Apr 22, 2024

CLA assistant check
All committers have signed the CLA.

@InsomniaCoder InsomniaCoder force-pushed the allow-cfn-promtail-parameterized-max-event-age branch from 0b94c3e to c874998 Compare April 22, 2024 15:08
@InsomniaCoder InsomniaCoder changed the title Parameterize the MaximumEventAgeInSeconds for lambda promtail CFN feat: parameterise the MaximumEventAgeInSeconds for prom-tail CFN Apr 22, 2024
@InsomniaCoder
Copy link
Contributor Author

I'll try to use this locally and share the results in the description once I have it.

@cstyan
Copy link
Contributor

cstyan commented Apr 22, 2024

This seems like a reasonable argument to the template, please ping me after you've tried it out 👍

@cstyan cstyan changed the title feat: parameterise the MaximumEventAgeInSeconds for prom-tail CFN feat: parameterise the MaximumEventAgeInSeconds for lambda-promtail CloudFormation template Apr 22, 2024
@InsomniaCoder
Copy link
Contributor Author

InsomniaCoder commented Apr 23, 2024

@cstyan I attached the result in the description. it seems to work. however, during the testing I also found that the log group is not a variable and it makes the creation failed. so, I have another change if it makes sense: #12750

Let me know what do you think, Thank you!

@cstyan
Copy link
Contributor

cstyan commented Apr 23, 2024

hey @InsomniaCoder lambda-promtail itself is best effort maintained currently, and the terraform and cloudformation files were always meant to be examples more than official "use exactly this to deploy" files, but I think both of your changes are still simple enough that we can merge them

@InsomniaCoder
Copy link
Contributor Author

@cstyan noted! from my side with these two PRs and probably a parameterized of the IAM role's name then it should be able to be used directly from my side.

but anyways, as you see fit. if you think they are beneficial to be merged, let me know if you need anything else 😄

@cstyan
Copy link
Contributor

cstyan commented Apr 23, 2024

@InsomniaCoder can you please include all the portions you want to parameterize in this PR 👍

@InsomniaCoder InsomniaCoder force-pushed the allow-cfn-promtail-parameterized-max-event-age branch from c8dabbf to 42d3e4a Compare April 24, 2024 09:15
@pull-request-size pull-request-size bot added size/S and removed size/M labels Apr 24, 2024
@InsomniaCoder
Copy link
Contributor Author

@cstyan combined all the changes.

image

thank you

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.

one last comment

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.

@InsomniaCoder thanks for your patience 👍

@cstyan cstyan changed the title feat: parameterise the MaximumEventAgeInSeconds for lambda-promtail CloudFormation template feat: parameterise the MaximumEventAgeInSeconds, LogGroupName, and IAMRoleName for lambda-promtail CloudFormation template Apr 27, 2024
@InsomniaCoder
Copy link
Contributor Author

@InsomniaCoder thanks for your patience 👍

thanks!!

@cstyan cstyan merged commit 8892dc8 into grafana:main Apr 29, 2024
58 checks passed
shantanualsi pushed a commit that referenced this pull request May 6, 2024
…MRoleName for lambda-promtail CloudFormation template (#12728)
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.

None yet

3 participants