feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT #1510

AlexeySoshin · 2023-11-10T13:28:04Z

Issue #, if available: #1444

Description of changes:

Adds support for POWERTOOLS_LOGGER_LOG_EVENT environment variable
The event will be logged only of this variable is set to true and @Logger(logEvent=true)

Checklist

Meet tenets criteria
Update tests
Update docs
PR title follows conventional commit semantics

Breaking change checklist

RFC issue #:

Migration process documented
Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jeromevdl · 2023-11-10T14:04:09Z

docs/core/logging.md

+| **functionMemorySize**  | String  | "128"                                                                                                |                                                                          |
+| **functionArn**         | String  | "arn:aws:lambda:eu-west-1:012345678910:function:example-powertools-HelloWorldFunction-1P1Z6B39FLU73" |                                                                          |
+| **xray_trace_id**       | String  | "1-5759e988-bd862e3fe1be46a994272793"                                                                | X-Ray Trace ID when Lambda function has enabled Tracing                  |
+| **function_request_id** | String  | "899856cb-83d1-40d7-8611-9e78f15f32f4""                                                              | AWS Request ID from lambda context                                       |


sorry to say that but can we avoid this reformatting? will be a mess with v2...

No big deal

jeromevdl · 2023-11-10T14:05:29Z

docs/core/logging.md


 ## Capturing context Lambda info

 You can enrich your structured logs with key Lambda context information via `logEvent` annotation parameter. 
 You can also explicitly log any incoming event using `logEvent` param. Refer [Override default object mapper](#override-default-object-mapper) 
 to customise what is logged.

+Note that the `logEvent` will work only if `POWERTOOLS_LOGGER_SAMPLE_RATE` environment variable is set to `"true"`


is it the python behaviour ?

I think it is either one or the other to have the event logged, not both

https://docs.powertools.aws.dev/lambda/python/latest/core/logger/#logging-incoming-event

This is the TypeScript behavior:
https://github.com/aws-powertools/powertools-lambda-typescript/blob/aa2673d89a6676f4d9242773543876464b3b65ef/packages/logger/src/Logger.ts#L904

We should follow python. It's either one or the other, objective is to simplify job, users choose either programatic way or with env var, we don't want to complexify and require both. More over, if we require both, it becomes a breaking change and should go in v2.

jeromevdl · 2023-11-10T14:07:17Z

docs/index.md

-| **POWERTOOLS_LOG_LEVEL** | Sets logging level | [Logging](./core/logging) |
-| **POWERTOOLS_TRACER_CAPTURE_RESPONSE** | Enables/Disables tracing mode to capture method response | [Tracing](./core/tracing) |
-| **POWERTOOLS_TRACER_CAPTURE_ERROR** | Enables/Disables tracing mode to capture method error | [Tracing](./core/tracing) |
+| Environment variable                   | Description                                                                            | Utility                   |


same here, can you avoid reformat, just add the new env var

I'll do a proper formatting in v2

jeromevdl · 2023-11-10T14:08:28Z

...core/src/main/java/software/amazon/lambda/powertools/core/internal/EnvironmentVariables.java

+
+public final class EnvironmentVariables {
+    public static class LOGGER {
+        public final static String LOG_EVENT = "POWERTOOLS_LOGGER_LOG_EVENT";


we already have other files containing environment variables, do not introduce a new one please

maybe that's something we should clean up in v2 by the way.

...re/src/main/java/software/amazon/lambda/powertools/core/internal/LambdaHandlerProcessor.java

jeromevdl · 2023-11-10T14:11:36Z

...ng/src/main/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspect.java

@@ -74,6 +76,12 @@ public final class LambdaLoggingAspect {
        }

        LEVEL_AT_INITIALISATION = LOG.getLevel();
+
+        if (System.getenv(EnvironmentVariables.LOGGER.LOG_EVENT) != null) {


please, remain consistent with the other environment variables line 67/68, we'll create a proper file in v2...

jeromevdl · 2023-11-10T14:12:32Z

...ng/src/main/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspect.java

@@ -74,6 +76,12 @@ public final class LambdaLoggingAspect {
        }

        LEVEL_AT_INITIALISATION = LOG.getLevel();
+
+        if (System.getenv(EnvironmentVariables.LOGGER.LOG_EVENT) != null) {
+            LOG_EVENT = Boolean.parseBoolean(System.getenv(EnvironmentVariables.LOGGER.LOG_EVENT));


twice getenv, can you use a variable?

jeromevdl · 2023-11-10T14:12:57Z

...ng/src/main/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspect.java

-        if (logging.logEvent()) {
+        // Make sure that the environment variable was enabled explicitly
+        // And that the handler was annotated with @Logging(logEvent = true)
+        if (LOG_EVENT && logging.logEvent()) {


Suggested change

if (LOG_EVENT && logging.logEvent()) {

if (LOG_EVENT || logging.logEvent()) {

jeromevdl

thanks for this PR, a few things to fix

jeromevdl · 2023-11-10T22:18:15Z

...rc/test/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspectTest.java

@@ -198,27 +182,24 @@ void shouldLogEventForHandler() throws IOException, JSONException {
        assertEquals(expectEvent, event, false);
    }

+    /**
+     * If POWERTOOLS_LOGGER_LOG_EVENT was set to false, the handler should log anything, despite @Logging(logEvent=true)


no, see my other comment

python is the ref, not typescript

…ogging for endpoints, even if the annotation doesn't specify it

jeromevdl · 2023-11-13T09:58:56Z

docs/core/logging.md

@@ -221,6 +221,8 @@ You can enrich your structured logs with key Lambda context information via `log
 You can also explicitly log any incoming event using `logEvent` param. Refer [Override default object mapper](#override-default-object-mapper) 
 to customise what is logged.

+If you want to enable this event logging for all handlers, set `POWERTOOLS_LOGGER_SAMPLE_RATE` environment variable to `"true"`


can we simplify this whole paragraph with the same content as python: https://docs.powertools.aws.dev/lambda/python/latest/core/logger/#logging-incoming-event

jeromevdl · 2023-11-13T10:00:47Z

...ng/src/main/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspect.java

+
+        String logEvent = System.getenv("POWERTOOLS_LOGGER_LOG_EVENT");
+        if (logEvent != null) {
+            LOG_EVENT = Boolean.parseBoolean(logEvent);


can we try catch if the parseBoolean fails and log a warn, to avoid an exception ?

We could with some effort, but I think this is before logger is initialized, though

jeromevdl

small changes and we should be good

docs/core/logging.md

…ent' into support_powertools_logger_log_event # Conflicts: # docs/core/logging.md

sonarcloud · 2023-11-14T10:38:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

* Setting up Kotlin environment. Converting test to Kotlin. * Deploying via SAM successfully. * Added Kotlin example. * Removing unused Gradle build file. * Adding SAM template so can be used as an existing project and Java target compatibility * Adding SAM template so can be used as an existing project * Updating guidance to use SAM for build and deploy * Restructuring separate Java and Kotlin examples. * Updating core examples readme to represent new structure for Java and Kotlin examples. * Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive * Updating to fix trailing \n * Updating guidance to be more specific for examples * Adopting new mechanism for specifying jvm target. * accommodating new project structure * Fixing link typo after refactoring * Setting up Kotlin environment. Converting test to Kotlin. * Deploying via SAM successfully. * Added Kotlin example. * Removing unused Gradle build file. * Adding SAM template so can be used as an existing project and Java target compatibility * Adding SAM template so can be used as an existing project * Updating guidance to use SAM for build and deploy * Restructuring separate Java and Kotlin examples. * Updating core examples readme to represent new structure for Java and Kotlin examples. * Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive * Updating to fix trailing \n * Updating guidance to be more specific for examples * Adopting new mechanism for specifying jvm target. * accommodating new project structure * Fixing link typo after refactoring * Flattening structure back to original to make merging easier for v2 * Adding build for Kotlin Gradle * Adding build for Kotlin Gradle - Restructuring Java examples to v1 approach * Correcting paths * Adding SNAPSHOT support and local capability for Maven. Testing using Java 1.8 * Reviewed and updated against PR comments. * Un-commenting examples * Adding validation step for IaC SAM * Adding Terraform for Java projects IaC validator and linter * Adding additional projects for SAM validation and matrix approach * Refactoring stream function to process input logging example with a Lambda Function URL instead of APIGW. * Demonstrating Java streaming response * Refactoring stream function to process input logging example to return * Update CONTRIBUTING.md * fix: get trace id from system property when env var is not set (#1503) * fix: check if XRAY Trace ID is present in System property * chore: remove erroneous extra char in tests * fix #1500 (#1506) * feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT (#1510) * chore: Addition of Warn Message If Invalid Annotation Key While Tracing #1511 (#1512) * feat: ALC (#1514) * handle AWS_LAMBDA_LOG configuration * ALC documentation + code review * update doc * chore:Prep release 1.18.0 (#1515) * chore:prep release 1.18.0 * update version * update version in kotlin example * maven local repo in gradle example * update changelog --------- Co-authored-by: scottgerring <scottgerring@users.noreply.github.com> * chore: update version to next snapshot: 1-19.0-SNAPSHOT (#1516) * update version to next snapshot: 1-19.0-SNAPSHOT * update version to next snapshot: 1-19.0-SNAPSHOT * update version to next snapshot: 1-19.0-SNAPSHOT * building only for LTS * Add some more margin to the test pause (#1518) * test: e2e tests with java 21 (#1517) * e2e tests with java 21 * Run Java21 tests using the Java17 compiler * Run all of the E2E tests in parallel, not just the first 3 * Try again * . * Let's try again * Add some comment on Java21 to the repo * Add caveat about lambda runtimes * Clean up wording a little --------- Co-authored-by: Scott Gerring <gerrings@amazon.com> * update doc for ALC (#1520) * chore: Testing java21 aspectj pre-release (#1519) * e2e tests with java 21 * use aspectj 1.9.21-SNAPSHOT * Fix log4j2.xml missing in logging test for java21 * rollback double runtime * remove comment * keep aspectj 1.9.7 in parent for java8 compatibility * use M1 instead of snapshot * update documentation for aspectj * update documentation for aspectj --------- Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com> * chore: Remove build cruft * Adding context for using RequestStreamHandler * removing pr_lint * Update examples/powertools-examples-core/sam/src/main/java/helloworld/AppStream.java Clarify usage of RequestStreamHandler. Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com> --------- Co-authored-by: Jason Harris <harrzjas@amazon.com> Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com> Co-authored-by: Jason Harris <harrzjas@amazon.co.uk> Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com> Co-authored-by: Michele Ricciardi <mriccia@amazon.com> Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com> Co-authored-by: jdoherty <jdoherty07@gmail.com> Co-authored-by: Scott Gerring <gerrings@amazon.com> Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com>

* docs: HelloWorldStreamFunction in examples fails with sam (#1532) * Setting up Kotlin environment. Converting test to Kotlin. * Deploying via SAM successfully. * Added Kotlin example. * Removing unused Gradle build file. * Adding SAM template so can be used as an existing project and Java target compatibility * Adding SAM template so can be used as an existing project * Updating guidance to use SAM for build and deploy * Restructuring separate Java and Kotlin examples. * Updating core examples readme to represent new structure for Java and Kotlin examples. * Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive * Updating to fix trailing \n * Updating guidance to be more specific for examples * Adopting new mechanism for specifying jvm target. * accommodating new project structure * Fixing link typo after refactoring * Setting up Kotlin environment. Converting test to Kotlin. * Deploying via SAM successfully. * Added Kotlin example. * Removing unused Gradle build file. * Adding SAM template so can be used as an existing project and Java target compatibility * Adding SAM template so can be used as an existing project * Updating guidance to use SAM for build and deploy * Restructuring separate Java and Kotlin examples. * Updating core examples readme to represent new structure for Java and Kotlin examples. * Refactoring application code for efficiency, updating build to cover tests too and is more idiomatic and readme to be more descriptive * Updating to fix trailing \n * Updating guidance to be more specific for examples * Adopting new mechanism for specifying jvm target. * accommodating new project structure * Fixing link typo after refactoring * Flattening structure back to original to make merging easier for v2 * Adding build for Kotlin Gradle * Adding build for Kotlin Gradle - Restructuring Java examples to v1 approach * Correcting paths * Adding SNAPSHOT support and local capability for Maven. Testing using Java 1.8 * Reviewed and updated against PR comments. * Un-commenting examples * Adding validation step for IaC SAM * Adding Terraform for Java projects IaC validator and linter * Adding additional projects for SAM validation and matrix approach * Refactoring stream function to process input logging example with a Lambda Function URL instead of APIGW. * Demonstrating Java streaming response * Refactoring stream function to process input logging example to return * Update CONTRIBUTING.md * fix: get trace id from system property when env var is not set (#1503) * fix: check if XRAY Trace ID is present in System property * chore: remove erroneous extra char in tests * fix #1500 (#1506) * feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT (#1510) * chore: Addition of Warn Message If Invalid Annotation Key While Tracing #1511 (#1512) * feat: ALC (#1514) * handle AWS_LAMBDA_LOG configuration * ALC documentation + code review * update doc * chore:Prep release 1.18.0 (#1515) * chore:prep release 1.18.0 * update version * update version in kotlin example * maven local repo in gradle example * update changelog --------- Co-authored-by: scottgerring <scottgerring@users.noreply.github.com> * chore: update version to next snapshot: 1-19.0-SNAPSHOT (#1516) * update version to next snapshot: 1-19.0-SNAPSHOT * update version to next snapshot: 1-19.0-SNAPSHOT * update version to next snapshot: 1-19.0-SNAPSHOT * building only for LTS * Add some more margin to the test pause (#1518) * test: e2e tests with java 21 (#1517) * e2e tests with java 21 * Run Java21 tests using the Java17 compiler * Run all of the E2E tests in parallel, not just the first 3 * Try again * . * Let's try again * Add some comment on Java21 to the repo * Add caveat about lambda runtimes * Clean up wording a little --------- Co-authored-by: Scott Gerring <gerrings@amazon.com> * update doc for ALC (#1520) * chore: Testing java21 aspectj pre-release (#1519) * e2e tests with java 21 * use aspectj 1.9.21-SNAPSHOT * Fix log4j2.xml missing in logging test for java21 * rollback double runtime * remove comment * keep aspectj 1.9.7 in parent for java8 compatibility * use M1 instead of snapshot * update documentation for aspectj * update documentation for aspectj --------- Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com> * chore: Remove build cruft * Adding context for using RequestStreamHandler * removing pr_lint * Update examples/powertools-examples-core/sam/src/main/java/helloworld/AppStream.java Clarify usage of RequestStreamHandler. Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com> --------- Co-authored-by: Jason Harris <harrzjas@amazon.com> Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com> Co-authored-by: Jason Harris <harrzjas@amazon.co.uk> Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com> Co-authored-by: Michele Ricciardi <mriccia@amazon.com> Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com> Co-authored-by: jdoherty <jdoherty07@gmail.com> Co-authored-by: Scott Gerring <gerrings@amazon.com> Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com> * deps: bump aspectj to 1.9.21 for jdk21 (#1536) * chore: SAM and Terraform IaC extracted from pr_build and simplified approach. (#1533) * SAM and Terraform IaC extracted from pr_build and simplified approach. * Update .github/workflows/pr_iac_lint.yml Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com> --------- Co-authored-by: Jason Harris <harrzjas@amazon.co.uk> Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.com> * chore: Remove empty CDK test (#1542) * Remove CDK test * Build core utilities so that sam validator can find them * Remove CDK test * Ok just build it all * Remove the plan ... * Remove terraform plan from iac lint --------- Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com> * dependabot on v2 branch (#1548) * chore: remove unecessary creds acquisition (#1572) * Merge from main --------- Co-authored-by: Jason Harris <jiharris90@gmail.com> Co-authored-by: Jason Harris <harrzjas@amazon.com> Co-authored-by: Jason Harris <harrzjas@amazon.co.uk> Co-authored-by: Jérôme Van Der Linden <117538+jeromevdl@users.noreply.github.com> Co-authored-by: Michele Ricciardi <mriccia@amazon.com> Co-authored-by: Alexey Soshin <alexey.soshin@gmail.com> Co-authored-by: jdoherty <jdoherty07@gmail.com> Co-authored-by: Jerome Van Der Linden <jeromevdl@gmail.com>

Add support for POWERTOOLS_LOGGER_LOG_EVENT

d5e3ee5

pull-request-size bot added the size/M label Nov 10, 2023

AlexeySoshin marked this pull request as draft November 10, 2023 13:28

Update documentation with POWERTOOLS_LOGGER_LOG_EVENT

d9b44ba

pull-request-size bot added size/L and removed size/M labels Nov 10, 2023

jeromevdl assigned AlexeySoshin Nov 10, 2023

jeromevdl reviewed Nov 10, 2023

View reviewed changes

...re/src/main/java/software/amazon/lambda/powertools/core/internal/LambdaHandlerProcessor.java Show resolved Hide resolved

jeromevdl reviewed Nov 10, 2023

View reviewed changes

jeromevdl requested changes Nov 10, 2023

View reviewed changes

jeromevdl changed the title ~~Feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT~~ feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT Nov 10, 2023

AlexeySoshin added 2 commits November 10, 2023 16:09

Set POWERTOOLS_LOGGER_LOG_EVENT in the intergration tests

3f6a477

Validate that the log file is not being written

288b676

jeromevdl reviewed Nov 10, 2023

View reviewed changes

AlexeySoshin added 5 commits November 13, 2023 09:43

Change the behaviour of POWERTOOLS_LOGGER_LOG_EVENT to force-enable l…

da1c37c

…ogging for endpoints, even if the annotation doesn't specify it

Remove EnvironmentVariables, to simplify v2 migration

03094f1

Remove formatting and change documentation

6a53b4c

Remove formatting

5df3b3b

Remove forceful logging of the events

d990ef8

AlexeySoshin marked this pull request as ready for review November 13, 2023 09:54

AlexeySoshin requested a review from jeromevdl November 13, 2023 09:54

jeromevdl reviewed Nov 13, 2023

View reviewed changes

AlexeySoshin commented Nov 13, 2023

View reviewed changes

docs/core/logging.md Outdated Show resolved Hide resolved

Update docs/core/logging.md

3e3b402

AlexeySoshin requested a review from jeromevdl November 13, 2023 10:15

AlexeySoshin added 2 commits November 14, 2023 10:36

Simplify documentation

4645b62

Merge remote-tracking branch 'origin/support_powertools_logger_log_ev…

93c3e69

…ent' into support_powertools_logger_log_event # Conflicts: # docs/core/logging.md

jeromevdl approved these changes Nov 14, 2023

View reviewed changes

jeromevdl merged commit de547d0 into aws-powertools:main Nov 14, 2023
10 of 17 checks passed

jasoniharris pushed a commit that referenced this pull request Nov 29, 2023

feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT (#1510)

7c51a38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT #1510

feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT #1510

AlexeySoshin commented Nov 10, 2023 •

edited by jeromevdl

Loading

jeromevdl Nov 10, 2023

AlexeySoshin Nov 13, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

AlexeySoshin Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl left a comment

jeromevdl Nov 10, 2023

jeromevdl Nov 10, 2023

jeromevdl Nov 13, 2023

jeromevdl Nov 13, 2023

AlexeySoshin Nov 13, 2023

jeromevdl left a comment

sonarcloud bot commented Nov 14, 2023

	if (LOG_EVENT && logging.logEvent()) {
	if (LOG_EVENT \|\| logging.logEvent()) {

feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT #1510

feat: Add support for POWERTOOLS_LOGGER_LOG_EVENT #1510

Conversation

AlexeySoshin commented Nov 10, 2023 • edited by jeromevdl Loading

Description of changes:

Breaking change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromevdl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromevdl left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 14, 2023

AlexeySoshin commented Nov 10, 2023 •

edited by jeromevdl

Loading