-
Notifications
You must be signed in to change notification settings - Fork 403
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(logger): add DatadogLogFormatter and observability provider #2183
feat(logger): add DatadogLogFormatter and observability provider #2183
Conversation
use_datetime_directive: bool = False, | ||
log_record_order: list[str] | None = None, | ||
utc: bool = False, | ||
use_rfc3339: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not change required, just a question: the only difference between this constructor and the base LambdaPowertoolsFormatter is this use_rfc3399
boolean that defaults to true
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be any downsides to making use_rfc3339
default to true if it were communicated well enough in release notes or the next major version?
Yeees! It’s rather silly isn’t it ;-)
…On Mon, 1 May 2023 at 15:14, Ruben Fonseca ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In aws_lambda_powertools/logging/formatters/datadog.py
<#2183 (comment)>
:
> +from typing import Any, Callable
+
+from aws_lambda_powertools.logging.formatter import LambdaPowertoolsFormatter
+
+
+class DatadogLogFormatter(LambdaPowertoolsFormatter):
+ def __init__(
+ self,
+ json_serializer: Callable[[dict], str] | None = None,
+ json_deserializer: Callable[[dict | str | bool | int | float], str] | None = None,
+ json_default: Callable[[Any], Any] | None = None,
+ datefmt: str | None = None,
+ use_datetime_directive: bool = False,
+ log_record_order: list[str] | None = None,
+ utc: bool = False,
+ use_rfc3339: bool = True,
Not change required, just a question: the only difference between this
constructor and the base LambdaPowertoolsFormatter is this use_rfc3399
boolean that defaults to true, right?
—
Reply to this email directly, view it on GitHub
<#2183 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBGIGUWP52AIK5DIHL3XD6ZLPANCNFSM6AAAAAAXRX7J3I>
.
You are receiving this because you authored the thread.Message ID:
***@***.***
com>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some small changes, good stuff!
|
||
|
||
class DatadogLogFormatter(LambdaPowertoolsFormatter): | ||
def __init__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some docstring explaining why this exist and what the small difference is to the base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're the best! Doing that now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Added a specific list so we can easily append if future changes are required - lemme know if that addresses it.
use_datetime_directive: bool = False, | ||
log_record_order: list[str] | None = None, | ||
utc: bool = False, | ||
use_rfc3339: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_rfc3339: bool = True, | |
use_rfc3339: bool = True, // This is the only change from the base class |
or something similar :) it helps future readers to spot the difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
we shall wait until GitHub or PyPi behaves well again ;) |
Figure I would give a community member thumbs up. I copied the |
Wonderful to hear, please keep up shouting out more :) |
Issue number: #2014
Summary
This PR add a new
DatadogLogFormatter
and documents the idea of Observability Provider.Changes
User experience
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.