-
Notifications
You must be signed in to change notification settings - Fork 141
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
RFC: Support for external observability providers - Logging #1500
Comments
Thanks for opening your first issue here! We'll come back to you as soon as we can. |
In response to @dreamorosi comment internally:
So the main issue is that Maybe can consider merging the persistent attributes and extra attributes in the Logger, and then passing |
So I think it can be beneficial to do a combination of the proposed solution I've suggested and the alternative solution provided, along with an additional change. To keep the processing of the var extraAttributes: LogAttributes = {};
if (typeof input !== 'string') {
extraAttributes = merge(extraAttributes, input);
}
extraInput.forEach((item: Error | LogAttributes | string) => {
const attributes: LogAttributes =
item instanceof Error
? { error: item }
: typeof item === 'string'
? { extra: item }
: item;
extraAttributes = merge(extraAttributes, attributes);
}); Then, we'll have public formatAttributes(attributes: UnformattedAttributes,
persistentLogAttributes?: LogAttributes,
extraAttributes?: LogAttributes): PowertoolLogItem; If we want to continue keeping the Regardless, I think rewriting the constructor for a One question I do have is how flexible the public formatAttributes(attributes: UnformattedAttributes,
persistentLogAttributes: LogAttributes,
additionalLogAttributes: LogAttributes): LogItem {
const baseAttributes: PowertoolLog = {
// standard attributes from UnformattedAttributes
cold_start: attributes.lambdaContext?.coldStart,
function_arn: attributes.lambdaContext?.invokedFunctionArn,
function_memory_size: attributes.lambdaContext?.memoryLimitInMB,
function_name: attributes.lambdaContext?.functionName,
function_request_id: attributes.lambdaContext?.awsRequestId,
level: attributes.logLevel,
message: attributes.message,
sampling_rate: attributes.sampleRateValue,
service: attributes.serviceName,
timestamp: this.formatTimestamp(attributes.timestamp),
xray_trace_id: attributes.xRayTraceId,
};
const powertoolLogItem = new LogItem({attributes: baseAttributes});
powertoolLogItem.addAttributes(persistentLogAttributes);
powertoolLogItem.addAttributes(additionalLogAttributes);
return powertoolLogItem;
} Let me know what you think of this solution! |
Hi Erika, thank you for the thoughtful discussion and also for taking in account my first round of internal comments. I like the direction in which the proposal is going and I think that the mixed proposal in your last comment is good. I also like both the ideas of expanding the
In theory, unless we decide to add new standard attributes, all the A of notes/question about the hybrid proposal, specifically around the last few lines: const powertoolLogItem = new LogItem({attributes: baseAttributes});
powertoolLogItem.addAttributes(persistentLogAttributes);
powertoolLogItem.addAttributes(additionalLogAttributes); I know that the proposal comes from the current implementation as well as the previous discussions, however now I'm starting to question what's the added value here in having the persistent & additional log attributes separate in the first place. Both types of attributes are user-provided attributes and the main/only differences about them are how long they are kept around and how they were added, which are concerns that involve the Logger class/utility but not necessarily the specific log item. Basically what I'm getting at is that at the time of formatting, which is right before emitting the log entry, these are just attributes and the log item doesn't care/have to know whether some of them will show up only in this entry / function invocation or in all entries. For the Relying on whether an attribute is "persistent" or "additional" to define the structure would create a strong coupling between how the logs are generated in the code & how they are formatted, and any inconsistency would result in two different logs. For instance, let's take these two cases: In the first case I define const logger = new Logger({
persistentLogAttributes: {
accountId: '1234'
},
});
logger.info('hi'); and then this second case, in which I pass const logger = new Logger();
logger.info('hi', { accountId: '1234' }); If I was defining a log formatter based on the types (i.e. I want all the persistent attributes in a certain place & the additional attributes in another), these two ways of instantiating the logger & emitting a log would result in two different JSON logs. In reality, in most cases you would want to define a structure based on the key & values, for instance: "as a customer I want the account id and function ARN, memory, etc. inside a key called const baseAttributes: PowertoolLog = {
// standard attributes from UnformattedAttributes
cold_start: attributes.lambdaContext?.coldStart,
functionInfo: {
function_arn: attributes.lambdaContext?.invokedFunctionArn,
function_memory_size: attributes.lambdaContext?.memoryLimitInMB,
function_name: attributes.lambdaContext?.functionName,
// NOTE HERE
accountId: extraAttributes?.accountId,
},
function_request_id: attributes.lambdaContext?.awsRequestId,
level: attributes.logLevel,
message: attributes.message,
sampling_rate: attributes.sampleRateValue,
service: attributes.serviceName,
timestamp: this.formatTimestamp(attributes.timestamp),
xray_trace_id: attributes.xRayTraceId,
}; If the two types of attributes are traded the same then I can do this easily. If instead we keep them separately in order to achieve something as trivial as the above would require me to handle conditionals (accountId: persistentLogAttributes?.accountId || additionalLogAttributes?.accountId,). To sum up: I think the Logger could continue to treat the two types as separate because they have different shelf-life, however once we get to the formatter they could potentially be joined together. What do you think? Please let me know if I misunderstood your proposal and/or I'm going off base. Also looping @am29d so he can start getting up to speed with the RFC & comment. Also, and finally, minor: since we are reworking this area, can we rename the |
The I'm on board with letting the formatter handle persistent and additional attributes together -- they'd be merged behind the scenes in Totally on board with renaming the Let me know if there's any more questions about the design, and @am29d, I look forward to working with you as well! |
Great to hear @erikayao93! Another thing I just realized is that currently when instantiating Then, every time we format the log item (aka emit a log) we remove the values that evaluate to Do you think this is something that we could move in the constructor, or even already in the Logger? I think doing this might speed up the attribute merging since there'd be potentially less keys to handle. And even if the merging operation stays similar in terms of perf, we there's no much value in keeping these empty keys around. What do you think? |
From what I can see about the behavior you just described, it's done through a member method in I could add the same method to the It's a little more difficult for cleaning up the base attributes, since they aren't a set of If we go forward with this design, do we still want to have that final check before we emit a log? |
Not sure, at least for the Powertools stack it'd be superfluous since we control everything. For custom formatters there's still a chance customers might format in a way that would result in attributes being undefined. So maybe I'm over complicating things and the initial design is fine. I propose that we keep this on the radar but make a decision once we start the actual implementation, I don't think it's a must and don't want to derail the conversation further (thanks for entertaining the thought tho!) |
Sounds good, I've updated the RFC to better match the design decisions we've talked about, let me know if I missed anything! I also cleaned up the alternative solutions and left a note about handling the empty keys there, just so we have a bit of documentation on that. |
Just realized I forgot to address the naming inconsistencies you brought up -- added a little section in the proposal for that now and updated the name everywhere I could find it! |
made tiny edits to enable syntax highlighting to ease reading
|
|
Is this related to an existing feature request or issue?
Powertools for AWS Lambda (Python) #2014, #1261, #646
Which Powertools for AWS Lambda (TypeScript) utility does this relate to?
Logger
Summary
Powertools for Python has implemented support for certain external observability providers out of the box for the Logger utility by extending the LogFormatter feature according to Powertools for AWS Lambda (Python) #2014. This allows customers using designated providers to use the Logger utility with either the default or a custom LogFormatter without having to design their own configurations to allow for integration with observability solutions other than CloudWatch.
In Powertools for TypeScript, the current LogFormatter lacks customizability on certain features, as described in issue #1261. By extending the capabilities of the LogFormatter to accommodate changes on all log features, we aim to better support third-party observability providers, starting with Datadog, and expanding to encompass the same list of providers as Powertools for Python.
Use case
The primary use case for this utility will be for customers who might want to extend the default LogFormatter to fully customize the logs emitted by Logger so that they can conform with their requirements or better work with supported AWS Lambda Ready Partners and AWS Partners observability providers.
Proposal
Terminology
Current LogFormatter Experience
Powertools for Typescript currently provides a base abstract class
LogFormatter
with the abstract methodformatAttributes
that must be implemented to define the formatting of base attributes:Our default implementation for the Logger uses the
PowertoolLogFormatter
class that builds on top of theLogFormatter
abstract class, maintaining its members and methods. The class provides the following implementation for theformatAttributes
method:This block of code provides default formatting for standard attributes and context-related ones if present. This formatted list of attributes is of type
PowertoolLog
, which is an alias for a specific list ofLogAttributes
.If customers wish to customize the formatting to meet their own requirements or to work with third-party observability providers, they can define a similar class to alter the formatting on these standard and context-related attributes:
Important to note here is the lack of functionality that is provided for altering persistent attributes or extra attributes. Instead, for persistent attributes added to the logger, it maintains the formatting that the user provided the attributes in. These are aggregated as another set of
LogAttributes
.Both sets of
LogAttributes
(one for the standard attributes and context-related ones and one for the persistent attributes) are combined in the constructor for aLogItem
, which has a private member attributes that is a list ofLogAttributes
.The benefit of the
LogItem
is that it also possesses methods for adding, getting, deleting, and setting attributes. However, theaddAttributes
method simply merges a new set oflogAttributes
intoattributes
oflogItem
. This means that customers do not have the ability to reorganize their persistent attributes with the formatted list of base attributes, nor do they have the ability to reformat the persistent attributes, and the list is simply appended to the end of the attributes list.For extra attributes, these attributes are converted into a set of
logAttributes
and added to theattributes
list with the following code:Similar to the persistent attributes, the
extraInput
formatting here does not provide customers with the ability to reformat or reorganize the attributes with their existing list of attributes for the log. The new attributes are merely appended to the end of the attributes list, after the base attributes and persistent attributes.Proposed PowertoolsLogFormatter Update
Firstly, to maintain naming consistency,
PowertoolLogFormatter
andPowertoolLog
will be renamed toPowertoolsLogFormatter
andPowertoolsLog
. If this inconsistency is discovered in other parts of the Logger utility through the implementation of this proposal, the name will also be updated in those instances.Instead of having persistent attributes and extra attributes merged into a
LogItem
after the base attributes have been formatted, we propose merging the persistent attributes and extra attributes first, into a set oflogAttributes
that can be passed intoformatAttributes
instead:This functionality will be maintained in the Logger file, where the
LogItem
used to be created.Then, to provide customers with access to formatting beyond the base attributes, we propose extending the definition of the base
LogFormatter
class and its formatAttributes method to access the original unformattedbaseAttributes
and the newadditionalLogAttributes
set:In order to maintain a mask on the merging functionality provided in the
LogItem
class, we will utilizeaddAttributes
, as defined inLogItem
, andformatAttributes
will be redefined to generate aLogItem
object rather thanLogAttributes
.Within the
PowertoolsLogFormatter
, theformatAttributes
function will be redefined in accordance to theLogFormatter
update to take in all attributes, not just the base attributes:We maintain the attributes of
PowertoolsLog
but maintain the customizability of adding fields the same way current functionality does.For a
LogItem
object to be created from this, we will have to redefine the constructor ofLogItem
to construct off a single attribute list. This will allowLogItem
to be more flexible in terms of its initial definition, while maintaining that other attributes can always be added or deleted later using its member functions.Custom LogFormatter Usage
If the customer would like to use another observability provider, or define their own logger functions, customers can define their own Formatter class that the customer can implement based on our existing framework and pass in to the Logger class. The class would implement the
formatAttributes
method similar to:If customers wish to, they can define their own
CustomLog
type to use as shown above that will pre-establish the fields ofattributeList
, similar to thePowertoolsLog
type that thePowertoolsLogFormatter
uses for the base attributes.Out of scope
Sending logs from Powertools to the customer's desired observability platform will not be in the scope of this project. The implementation should only support modifying the output of the Logger so that the customer can push them to their platform of choice.
Potential challenges
These proposed changes will likely result in a breaking change, which will require a major release and a migration plan/guide.
By reorganizing the PowertoolsLogFormatter, some functionality that is currently contained in LogItem may be removed or migrated to the Formatters. We will have to identify other dependencies of the LogItem class to ensure that other functionality is not lost.
We need to determine which platforms we want to support out-of-the-box.
Dependencies and Integrations
We will have to integrate with (and thus, have a dependency on) the platforms we decide to support out-of-the-box. For now, we will be focusing on Datadog, but this should be expanded to encompass the same providers supported by Powertools for Python, listed here.
Alternative solutions
To optimize the merging of
LogAttributes
sets, we can consider migrating theremoveEmptyKeys()
method fromLogItem
to be used on small sets of attributes before they are merged together.We have considered several different ways of handling passing the persistent attributes and extra attributes to the formatter:
LogAttributes
to pass intoformatAttributes
LogAttributes
, so that customers can merge them into their log however they wishLogger
as much as possible, without encroaching on the more straightforward functionality for the formatterAcknowledgment
Future readers
Please react with 👍 and your use case to help us understand customer demand.
The text was updated successfully, but these errors were encountered: