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

feat: advanced logging #1539

Merged
merged 14 commits into from
Feb 28, 2024
Merged

feat: advanced logging #1539

merged 14 commits into from
Feb 28, 2024

Conversation

jeromevdl
Copy link
Contributor

Issue #, if available: #965 #1508

Description of changes:

  • Removed the message logging in JSON, message will remain a simple String
  • If people want to add custom fields, they can use either
  • Events and responses are now logged using StructuredArguments and not in the message anymore
  • Simplified the Json serialization in logback module
  • Breaking Change: updated the JsonConfig ObjectMapper to enable/disable some features:
    • do not fail on unknown properties
    • ignore null fields ==> this breaks idempotency compatibility (hash is different now)
    • order fields alphabetically

Checklist

Breaking change checklist

Breaking change on the Jackson ObjectMapper that now comes with default config (ignore null fields)

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 jeromevdl marked this pull request as draft December 15, 2023 22:44
Copy link

github-actions bot commented Dec 15, 2023

💾 Artifacts Size Report

Module Version Size (KB)
powertools-common 2.0.0-SNAPSHOT 9.59
powertools-serialization 2.0.0-SNAPSHOT 18.22
powertools-logging 2.0.0-SNAPSHOT 33.07
powertools-logging-log4j 2.0.0-SNAPSHOT 20.60
powertools-logging-logback 2.0.0-SNAPSHOT 17.02
powertools-tracing 2.0.0-SNAPSHOT 14.00
powertools-metrics 2.0.0-SNAPSHOT 14.05
powertools-parameters 2.0.0-SNAPSHOT 17.46
powertools-validation 2.0.0-SNAPSHOT 20.80
powertools-cloudformation 2.0.0-SNAPSHOT 16.99
powertools-idempotency-core 2.0.0-SNAPSHOT 35.55
powertools-idempotency-dynamodb 2.0.0-SNAPSHOT 12.90
powertools-large-messages 2.0.0-SNAPSHOT 17.45
powertools-batch 2.0.0-SNAPSHOT 16.89
powertools-parameters-ssm 2.0.0-SNAPSHOT 10.69
powertools-parameters-secrets 2.0.0-SNAPSHOT 9.90
powertools-parameters-dynamodb 2.0.0-SNAPSHOT 11.94
powertools-parameters-appconfig 2.0.0-SNAPSHOT 11.98

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (v2@f625b33). Click here to learn what that means.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@          Coverage Diff          @@
##             v2    #1539   +/-   ##
=====================================
  Coverage      ?   78.32%           
  Complexity    ?      593           
=====================================
  Files         ?       66           
  Lines         ?     2265           
  Branches      ?      319           
=====================================
  Hits          ?     1774           
  Misses        ?      373           
  Partials      ?      118           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

};

private final EventResolver internalResolver;

private static final Map<String, EventResolver> eventResolverMap = Stream.of(new Object[][] {
private static final Map<String, EventResolver> eventResolverMap = Collections.unmodifiableMap(Stream.of(new Object[][] {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move to Java 11 to declare this more easily... ? :(

@scottgerring scottgerring self-requested a review December 18, 2023 09:08
Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial feedback - I believe the approach makes sense and represents an improvement to the UX. I would love to review the whole thing once you've done the docs too.

@jeromevdl jeromevdl self-assigned this Jan 8, 2024
Copy link

sonarcloud bot commented Jan 8, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

5 New issues
0 Security Hotspots
No data about Coverage
0.6% Duplication on New Code

See analysis details on SonarCloud

@scottgerring scottgerring marked this pull request as ready for review January 24, 2024 08:34
Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some doc feedback

@@ -563,35 +444,35 @@ we provide [built-in JMESPath expressions](#built-in-correlation-id-expressions)

#### Custom keys

???+ warning "Custom keys are persisted across warm invocations"
Always set additional keys as part of your handler method to ensure they have the latest value, or explicitly clear them with [`clearState=true`](#clearing-state).
** Using StructuredArguments **
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Appending Additional Keys" ? So that the heading reflects when you can learn to do, not a detail of how to do it


** Using MDC **

Mapped Diagnostic Context (MDC) is essentially a Key-Value store. It is supported by the [SLF4J API](https://www.slf4j.org/manual.html#mdc){target="_blank"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key-value, no need to capitalise


### Removing additional keys

You can remove additional keys added with the MDC using `MDC.remove("key")`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Typically, you'd remove additional keys at the end of your request handler, so they do not apply to subsequent requests"


### Removing additional keys

You can remove additional keys added with the MDC using `MDC.remove("key")`.

#### Clearing state

Logger is commonly initialized in the global scope. Due to [Lambda Execution Context reuse](https://docs.aws.amazon.com/lambda/latest/dg/runtimes-context.html){target="_blank"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logger is


#### Clearing state

Logger is commonly initialized in the global scope. Due to [Lambda Execution Context reuse](https://docs.aws.amazon.com/lambda/latest/dg/runtimes-context.html){target="_blank"},
this means that custom keys can be persisted across invocations. If you want all custom keys to be deleted, you can use
this means that custom keys, added with the MDC can be persisted across invocations. If you want all custom keys to be deleted, you can use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalisation "This"

@@ -1071,33 +1005,6 @@ If you need to customize format and timezone, you can change use the following:
</encoder>
```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more custom object mapper?

Copy link

sonarcloud bot commented Feb 28, 2024

Quality Gate Passed Quality Gate passed

Issues
5 New issues

Measures
0 Security Hotspots
No data about Coverage
0.6% Duplication on New Code

See analysis details on SonarCloud

@scottgerring scottgerring merged commit 7dff30c into v2 Feb 28, 2024
18 checks passed
@scottgerring scottgerring deleted the feat/advanced-logging branch February 28, 2024 12:29
@jeromevdl
Copy link
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants