search

Graylog2 / graylog-s3-lambda

An AWS Lambda function that reads logs from S3 and sends them to Graylog
GNU General Public License v3.0
12 stars 6 forks source link

S3 Lambda function - Initial version #4

Closed danotorrey closed 4 years ago

danotorrey commented 5 years ago

When reviewing, please start at https://github.com/Graylog2/graylog-s3-lambda/blob/initial-version/src/main/java/org/graylog/integrations/s3/GraylogS3Function.java and follow the logic down from there.

This is hard to test without the documentation, so a first-pass code review would be a great place to start.

danotorrey commented 5 years ago

The GELF client files will be removed and replaced with the GELF client dependency once the message flushing on shutdown change https://github.com/Graylog2/gelfclient/pull/43 is merged and released.

danotorrey commented 5 years ago

@bernd Thanks for the feedback. I agree with everything here. I have a few more points to address in the morning.

danotorrey commented 4 years ago

@bernd All suggestions have been addressed.

I have one remaining concern: Would we ever need to support a different delimiter other than new lines? Do we need to account for this in this release? I don't think so. New-line delimited is probably an OK default. We could add a new config variable if we ever need to support different delimiters.

danotorrey commented 4 years ago

@bernd Once we release the GELF client, I can swap the classes with the dependencies here.

bernd commented 4 years ago

I have one remaining concern: Would we ever need to support a different delimiter other than new lines? Do we need to account for this in this release? I don't think so. New-line delimited is probably an OK default. We could add a new config variable if we ever need to support different delimiters.

@danotorrey Agreed! Let's think about that once we need it. :smiley:

danotorrey commented 4 years ago

@bernd Wow, jadconfig is really really nice!! I shall never hand-parse config again.

I did a smoke test and all looks good. Once the gelfclient release artifact is up, I'll switch to it in this project, do one more test, then can merge this.

danotorrey commented 4 years ago

@bernd This is ready for final review. I have dropped the GELF sources and added the dependency. I have also added a user-definable logging level https://github.com/Graylog2/graylog-s3-lambda/pull/4/commits/eab1268ff02500ef43b5df085fcbc9b5eedd1b96.

My testing passes so far. If all looks good to you, then we can release this version after merging this PR.