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

fix(logger): enable logging of arbitrary objects #883

Merged
merged 5 commits into from May 24, 2022
Merged

fix(logger): enable logging of arbitrary objects #883

merged 5 commits into from May 24, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 18, 2022

Description of your changes

This PR relaxes the restrictions introduced by #614. In #565, we decided to restrict the additional logging data objects in their shape. It turned out, that this restriction prevents from logging of arbitrary objects, which is inconvenient.
In this PR, the type restrictions are removed, so it is now possible to specify any object as additional payload when calling the logger methods directly.

How to verify this change

  • Run unit tests
  • Run E2E tests
  • Review docs and examples
  • Try to log an arbitrary object by providing it as a property of a string-indexed object, e.g.
import { Logger } from '@aws-lambda-powertools/logger';

const logger = new Logger({ logLevel: 'DEBUG' });

export const handler = async (event) => {
  logger.debug('Lambda invoked', { details: { event } });
};

No compile time errors should be generated for the above example. The code should generate a valid log entry when executed.

Related issues, RFCs

#880

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary 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.

@ghost
Copy link
Author

ghost commented May 18, 2022

@dreamorosi Ready for review

@github-actions github-actions bot added the bug Something isn't working label May 18, 2022
@dreamorosi dreamorosi linked an issue May 18, 2022 that may be closed by this pull request
@dreamorosi dreamorosi added this to the production-ready-release milestone May 18, 2022
@dreamorosi dreamorosi assigned ghost May 18, 2022
docs/core/logger.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Looking good, I'm going to run the integration tests on this branch: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/2347203386

@flochaz
Copy link
Contributor

flochaz commented May 19, 2022

Can you add such object log in the e2e tests as well ?

Dmitry Balabanov added 2 commits May 19, 2022 10:24
@dreamorosi
Copy link
Contributor

Copy link
Contributor

@saragerion saragerion left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR!
I have some questions about the choice of making the types more loose than they were before.
While making the types very loose (unknown is the most loose type we can use), we can still keep the existing types and allow for nested objects to be added anyway.
If there are some struggle with making the types work (I know they can be a pain :-)), I am happy to support/ help out myself with those and improve the existing types, rather than using unknown.

Also I would like us to cover a more diverse scenarios that include nested objects with different types next to the one where the object value is a string.

packages/logger/tests/unit/Logger.test.ts Outdated Show resolved Hide resolved
docs/core/logger.md Show resolved Hide resolved
packages/logger/src/types/Log.ts Show resolved Hide resolved
@dreamorosi
Copy link
Contributor

E2E test is failing (link), it seems for a missing module (so before running the actual tests).

We need to investigate further.

@ghost
Copy link
Author

ghost commented May 19, 2022

@dreamorosi I haven't changed anything in the modules. In fact, we already had a similar issue with my first PR. The issue was caused by a faulty commit on main I based my branch on. Should I rebase my branch on the latest main again?

@dreamorosi
Copy link
Contributor

@dreamorosi I haven't changed anything in the modules. In fact, we already had a similar issue with my first PR. The issue was caused by a faulty commit on main I based my branch on. Should I rebase my branch on the latest main again?

Yes, please, let's try to rebase.
And yes, I see that there are no changes in the modules.

@ghost
Copy link
Author

ghost commented May 19, 2022

@dreamorosi I double checked and can confirm that my branch is based on the latest main. In my fork, the tests are passing on my branch, please see the test run. Could you please check if something is going wrong on checkout/merge?

@ghost
Copy link
Author

ghost commented May 20, 2022

@saragerion Thanks for your comments! I replied directly in the threads. I also updated the tests to operate on complex and nested objects of various types.

To your main question: why unknown? I have not found any other way to allow logging of objects typed via arbitrary interfaces. Since we can't control how users define their interfaces (neither how AWS SDK does this), we would not be able to come up with a type definition that covers all those interfaces. A possible solution would be an empty interface, but it would actually mean unknown as well. Researching generics as suggested by @dreamorosi hasn't worked out either. If there's a trick in TypeScript I'm not aware of, please let me know!

unknown is very permissive, that is true. However, if we want to allow logging of arbitrary objects as stated in the issue, this is probably the way to go. Users could define some types we haven't thought of.

The latest E2E test run is passing.

@dreamorosi dreamorosi self-requested a review May 24, 2022 12:04
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this topic, really appreciate it!

@dreamorosi dreamorosi added the logger This item relates to the Logger Utility label May 24, 2022
@saragerion saragerion merged commit 5d34854 into aws-powertools:main May 24, 2022
@ghost ghost deleted the fix/log-arbitrary-objects branch May 24, 2022 14:41
flochaz pushed a commit that referenced this pull request Jun 3, 2022
* fix(logger): enable logging of arbitrary objects

* Update docs

* Add E2E test

* Use nested objects in tests

* Update usage docs
dreamorosi pushed a commit that referenced this pull request Aug 2, 2022
* fix(logger): enable logging of arbitrary objects

* Update docs

* Add E2E test

* Use nested objects in tests

* Update usage docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working logger This item relates to the Logger Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: regression - not possible to log arbitrary objects
3 participants