powertools-lambda-typescript icon indicating copy to clipboard operation
powertools-lambda-typescript copied to clipboard

feat(logger): introduce log key reordering functionality

Open arnabrahman opened this issue 1 year ago • 4 comments

Summary

By default, we can not change the log key positions. This PR gives the user the ability to change log key positions.

Changes

  • This is a WIP PR
  • Mostly followed the recommendations from the comment from the actual issue.
  • A new option logRecordOrder is added for reordering.
  • Change formatAttributes function for the new option.
  • For now logRecordOrder holds the keys of UnformattedAttributes type
  • But looking at the python powertools doc, i believe we should type the option for standard structured keys. As far as i understand PowertoolsLog holds the type to the structured keys mentioned in the doc. Please correct me if i am wrong.

Issue number: #1568


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.

arnabrahman avatar Jul 05 '24 08:07 arnabrahman

Hi @arnabrahman, thank you for opening this PR!

I'll be reviewing it this morning and will get back to you, but overall I think we're going in the right direction.

dreamorosi avatar Jul 08 '24 07:07 dreamorosi

i believe we should type the option for standard structured keys

I was under the same impression, but looking at the docs again I think we're mistaken, since the docs mention this:

You can change the order of standard Logger keys or any keys that will be appended later at runtime

Additionally, in the example they show usage of a key (request_id) that is not part of the standard keys and that is added at runtime.

With this in mind I think we should follow the same pattern, if possible.

I haven't tested this in the code, but at first glance if I remember correctly, I think we should be able to include all keys to the new logic by working with the additionalLogAttributes (of type LogAttributes) object.

Doing this will mean we probably remove this line, which I'm not sure if it'll have any impact on the log output.

https://github.com/aws-powertools/powertools-lambda-typescript/blob/d76eb1d3d1569bd4675f2ff74c909b5b3976e232/packages/logger/src/formatter/PowertoolsLogFormatter.ts#L39

If this is a viable option, we should also modify the types of the new logRecordOrder accordingly (maybe an union between the keys of UnformattedAttributes and LogAttributes?).


Let me know if I cleared your questions 😄

dreamorosi avatar Jul 08 '24 07:07 dreamorosi

This summer felt like an eternity but good to be back.

I was looking at your suggestions @dreamorosi, on a high level it should be possible to include all the additional keys although I have to look a bit more to validate this. But first I have to merge the main branch but I cannot. I am getting pre-commit error for markdownlint-cli2 in packages/tracer/README.md. I ran setup-local & tried again but still got the same result. Is there anything that needs to be done?

Screenshot from 2024-08-25 16-19-59

arnabrahman avatar Aug 25 '24 10:08 arnabrahman

Hi @arnabrahman, glad to hear from you again! I hope you had a great summer and some well deserved rest 😃

The errors in the screenshot are strange because the changelog file with issue should be ignored from the markdown lint (here).

I will look into it as I have a rough idea of why this is happening, but for now I think it's safe to run the commit with the --no-verify flag, so you can skip the local checks for this one time. Either way they'll run again here in the PR. Sorry for the confusion!

dreamorosi avatar Aug 26 '24 09:08 dreamorosi

@dreamorosi So i have progressed well on this. But I have another question. Right now the logRecordOrder passes down to child logger. What if we want to override logRecordOrder to child instance?

Currently while creating child, logFormatter is passed to child instance. https://github.com/aws-powertools/powertools-lambda-typescript/blob/main/packages/logger/src/Logger.ts#L308

And logFormatter is not reinitialized if it's already set https://github.com/aws-powertools/powertools-lambda-typescript/blob/main/packages/logger/src/Logger.ts#L1085

In this case, if we provide logRecordOrder to child, it will not be overridden. Correct me if I am wrong

arnabrahman avatar Sep 02 '24 16:09 arnabrahman

In this case, if we provide logRecordOrder to child, it will not be overridden. Correct me if I am wrong

You are correct, we reuse the same log formatter from the parent because up until now it was reasonable to do so.

For the time being I would opt for keeping the logic as-is and prevent customers from overriding logRecordOrder when creating a child.

This means that child loggers will have the same log formatter and log record order as their parent. If there's demand for this feature we can easily add it in the future, but for now I'd simply go ahead as-is.

dreamorosi avatar Sep 02 '24 17:09 dreamorosi

Hi @arnabrahman, thank you for the PR - I've started reviewing it and should be able to finish by tomorrow morning at the latest.

dreamorosi avatar Sep 04 '24 08:09 dreamorosi