Adding features to control if the macro uses env_logger or tracing and allow more flexibility around logging initialization/configuration

[JeremieRodon]

Pull Request

Description

Please include a summary of the changes and which issue is being fixed.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Other (please describe):

Testing

The test suite ran successfully.
Tests had been added for the new log_init feature.

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

Breaking change: the lambda payload and AppsyncEvent will no longer be logged by default.

[dfeyer]

Thanks for the PR It works nicely. I can provide my own tracing init with no problem.

I'm a bit concerned about the default logging of the full LambdaEvent... I think this shouldn't be your lib's responsibility, at least not by default. Lambda events can contain sensitive information (API keys, bearer tokens, personal information), and having this information in the logs by default is a red flag for many organizations.

[JeremieRodon]

In fact, there is 3 new feature flags that comes with this pull request (as mentioned):

• log => make it so the macro cares about logging at all and log things using the log crate
• env_logger => exposes and initialize the env_logger crate (imply the log feature), this feature is the default one and produces the same behavior as the current crate version.
• tracing => exposes and initialize the tracing and tracing_subscriber crates, and instrument the macro-generated function_handler and appsync_handler functions (imply the log feature)

In addition to the log_init = fn_name option for the appsync_lambda_main!() proc macro, if the log feature is enabled you also have the following option:

• event_logging = bool: If true, the macro will generate code to dump the entire event JSON in the logs (default: true)

My rational here is 1/ not cause a breaking change and 2/ I believe most of the time you do want to log the entire event. I hear you when you say this should not be the responsibility of my lib, but I respectfully disagree in this particular case: as I propose a procedural macro generating the lambda main and boilerplate, I'm the only one who can log the event. That being said, if you have a Lambda function handling secrets, you can disable the event dump.

So the way I see it:

• If you don't care at all about the macro-generated boiler plate logging anything or exposing any log-related crate, just use default-features = false and do what you want
• If you want it to log, but use a backend other than env_logger or tracing, use default-features = false and enable the log feature
• If you want to expose and get reasonable defaults for env_logger, nothing to do, that's the current and immediate futur default behavior
• If you want to expose and use tracing, use default-features = false and enable the tracing feature.

NB: Currently, the log_init = fn_name option is also tied to the log feature, but I realized while writing this that it makes sense to provide it even if no feature is enabled, as it is the only way to insert user-provided code in the main function. So I will update the PR.

[dfeyer]

event_logging = bool: If true, the macro will generate code to dump the entire event JSON in the logs (default: true)

I would say, from a security standpoint, that this should be false by default and let the user decide. And actually, the log value is INFO. From my experience, it should be DEBUG. Having this in production can be costly, as filling up the logs can be. It's always too late when you discover that your 90 days of logs are full of PI information or other sensitive information.

Maybe we can rename log_init to a more generic init and pass the LambdaEvent to it, so the user of the lib has full freedom to log what they want. What do you think about that?

[dfeyer]

Regarding the issue with the inline JSON string in the log.

tracing's standard macros don't natively support logging serde_json::Value as structured nested JSON. The {} and {:?} formatters always produce strings. A possible solution will be to use tracing's valuable feature (if integrated). This requires the valuable feature and Valuable trait implementation.

[JeremieRodon]

Maybe we can rename log_init to a more generic init and pass the LambdaEvent to it

Unfortunately that's impossible, because the log_init code must be called in the main function (it must only run once), while the LambdaEvent is only known in the function_handler function (invoked for each event the runtime receive).

Then there is already the hook option that allow you to give a async fn(&AppsyncEvent<Operation>) -> Option<AppsyncResponse>. I primarily intended it as a custom auth mecanism, but it has access to each individual event and can print them in debug. It won't be able to serialize as JSON though, because I did not implement Serialize for the AppsyncEvent type.

I could add another option, maybe named (e.g.) payload_inspector that could take a fn(&serde_json::Value) and get the Lambda payload before it is deserialized (currently, the payload is logged if the log feature is enabled and event_logging = true ). That will be for another PR though.

I would say, from a security standpoint, that this should be false by default and let the user decide.

Yeah you are right I can't argue against that. I suppose a mild breaking change to modify that would be acceptable. So it is done.

A possible solution will be to use tracing's valuable feature (if integrated).

And do you feel it is necessary to implement that? If yes, can you point me to existing example or describe what you expect?

[JeremieRodon]

@dfeyer At this point I believe I'm done for this particular PR and your request for tracing support. The PR is big enough for my tastes and it will be a squash&merge so I'd rather keep it as it is. Ok?

About Valuable and payload_inspector, let's open dedicated issues for that if you feel its necessary (I think I will do the payload_inspector anyway in the coming weeks).

[dfeyer]

Ok for me and thanks for this PR and the discussions