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

Suggestion: Add Structured logging #76

Closed
therealvio opened this issue Jul 20, 2023 · 8 comments
Closed

Suggestion: Add Structured logging #76

therealvio opened this issue Jul 20, 2023 · 8 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@therealvio
Copy link

Hi team!

We are looking to enhance the log outputs of alterNAT Lambdas in the form of structured logging. This would tremendously improve the searchability and analysis of logs when errors surface.

For those not privy to structured logging, check out this article.

Would the maintainers of this repo be open to a PR that featured structured logging? We'd be happy to implement it!

Implementation questions:

  • Would you prefer this as something that users opt in/opt out of? Or would you prefer to implement structured logging as the only logging solution?
  • Do you have any preference on what library/libraries that would be used for this? I was looking to implement structlog (https://www.structlog.org/en/stable/ & https://github.com/hynek/structlog) as a potential library. It has a fair amount of stars, and is in active development.

I have read through the Contributing, and Code of Conduct documentation for the repo, though I am also open to suggestions, or guidelines you may have that hasn't yet been covered in the documentation.

Thanks!

@bwhaley
Copy link
Member

bwhaley commented Jul 20, 2023

Great suggestion, makes sense to me. structlog seems like a good approach.

Would you prefer this as something that users opt in/opt out of? Or would you prefer to implement structured logging as the only logging solution?

Let's use this as the only logging solution. We'll cut a new minor version release and mark it as backward incompatible.

Do you have any preference on what library/libraries that would be used for this? I was looking to implement structlog (https://www.structlog.org/en/stable/ & https://github.com/hynek/structlog) as a potential library. It has a fair amount of stars, and is in active development.

Structlog seems widely used/accepted so let's go with it. I asked around and nobody else had a strong preference.

@therealvio
Copy link
Author

I neglected to explain the why, so I will add here:

The intention is to produce clear signals on errors and success messages on connection reporting. If users setup SLOs around the error messages rather than downstream events (like how long a NAT gateway was used), it would make SLO reporting more accurate.

@bwhaley
Copy link
Member

bwhaley commented Jul 21, 2023

Makes sense. I could see a number of use cases, from connection success monitoring to failover event monitoring.

@bwhaley bwhaley added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jul 27, 2023
@bwhaley
Copy link
Member

bwhaley commented Sep 11, 2023

Closing this since it hasn't been active in a while. Do feel free to submit a PR if you'd like to see this enhancement!

@bwhaley bwhaley closed this as completed Sep 11, 2023
@therealvio
Copy link
Author

therealvio commented Jan 15, 2025

Hey gang,

I am writing to let you know that I am taking a look at this now.

This is my first time using AWS SAM (we typically run the Lambda containers ourselves via docker-compose), so please forgive any ignorance on my end with using it. I managed to get a bare-minimum local test working, though only for the connectivity testing. So my question is:

When running the AutoScalingTerminationFunction test, there's attempts at trying to manipulate infrastructure. So my question is: when you run your tests, do you have the infrastructure stood up beforehand? Or am I missing a step to mock otherwise "real" responses?

Thanks!

@therealvio
Copy link
Author

Another thing too: with regards to [this comment](#76 (comment), this would be a seperate PR since it would involve some changes in how the reporting for aws-alternat works.

Shall I raise a separate issue for this before breaking out another PR for this @bwhaley?

@therealvio
Copy link
Author

PR is up!
#122

@therealvio
Copy link
Author

PR for exposing the successful connection logs is also up:
#123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants