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: Add support for WAF logs in S3 #10416
lambda-promtail: Add support for WAF logs in S3 #10416
Conversation
ee38354
to
77011d1
Compare
Used #8254 as a guide which was unfortunately closed without merging. |
To not lose nanosecond precision. Also added comments to the function.
@cstyan Thank you for your comments, added commits to address them. |
|
||
fractionalSec := float64(i % sec) | ||
if fractionalSec == 0 { | ||
return sec, 0, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want to set an actual value for err
here? otherwise it has the nil value from strconv.ParseInt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, err
would be nil if fractionalSec
is 0. There would only be an error during parsing, fractionalSec
being 0 just means that the Unix time only has seconds resolution. It is not an error.
@@ -652,64 +682,71 @@ func TestProcessSQSEvent(t *testing.T) { | |||
require.True(t, handlerCalled) | |||
} | |||
|
|||
func TestToMicroseconds(t *testing.T) { | |||
func TestGetUnixSecNsec(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test needs a case for the fractionalSec == 0
case as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests fractionalSec == 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just need to update based on main one last time I think
Sorry, just saw a typo in the test; it was referring to the old function name. Pushed a fix. |
**What this PR does / why we need it**: Adds support for WAF logs in S3 in lambda-promtail **Which issue(s) this PR fixes**: Fixes #<issue number> **Special notes for your reviewer**: **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [x] Tests updated - [x] `CHANGELOG.md` updated - [x] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [x] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [x] 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](grafana@d10549e)
What this PR does / why we need it:
Adds support for WAF logs in S3 in lambda-promtail
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updatedadd-to-release-notes
labeldocs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PR