feat(logger): pretty printing logs in local and non-prod environment

[shdq]

Description of your changes

Changes increase JSON.stringify indentation to 4 if the POWERTOOLS_DEV env variable has a truthy value.

Feature described and discussed in #751

How to verify this change

Related issues, RFCs

RFC aws-powertools/powertools-lambda#86

Issue number: #751

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.

[shdq]

Hello @dreamorosi.

I forgot about the docs, so I'm going to commit and push it tomorrow. And we can discuss current changes until then.

[dreamorosi]

Hi @shdq thanks a lot for the code contribution, this is looking promising :)

I have left a comment around the placement/evaluation order of the isDevMode/INDENTATION values in packages/logger/src/Logger.ts. I would like to hear your opinion on the idea.

Another point that I would like to bring on the table is whether or not the changes currently applied under packages/logger/src/config/* should stay there or they should be placed under packages/commons/src/config.

On one hand, at least for now, the only utility to use this variable & this logic from the EnvironmentVariablesService would be Logger. So having the logic under this package makes sense.

On the other hand, the original RFC talks about the POWERTOOLS_DEV variable influencing the behavior of multiple utilities, some of which don't exist today in this project.

One could argue that in order to be future proof, the logic could be placed today in the EnvironmentVariablesService that is present in commons (link) and that is extended by all other utilities.

Having the logic already in the commons package would allow to easily integrate the feature to other utilities in the future, however this might be a case of premature optimization since we don't know if/when these features will land and until then users who use only the Metrics or Tracer utilities will be having extra unused code as dependency.

Would be keen to hear your opinion on the topic.

[shdq]

packages/logger/src/config vs packages/commons/src/config

I would stick to the RFC since there is a decision made that POWERTOOLS_DEV will influence multiple utilities, so I think the best place for it is in packages/commons/src/config. I don't see it as premature optimization since it is a shared setting, but the trade-off is the timeframe of adding support of POWERTOOLS_DEV into other packages.

When I wrote the plan in the issue I mentioned that I didn't find a "common" place. My fault that I didn't sync and see the recent change #1123 that you moved the config to commons. So I would have reconsidered the decision.

[dreamorosi]

Hi @shdq thank you for the changes, I'll wait for your confirmation before reviewing again.

[shdq]

@dreamorosi hello! If we stick to packages/logger/src/config it is ready to review.

[dreamorosi]

Okay, let's keep it as it is now and if we add similar features to other utilities we'll move this logic in commons then.

I've already done a first pass of review and I see that the tests are passing. Before marking it as approved I'd like to test it locally to see how it would look from an user perspective. I'll try to do that by Monday end of morning (CEST).

In the meantime I'll also ask for a second review from other maintainers.

Thank you!

[dreamorosi]

I have built the package from this branch & tested the following dummy lambda code:

import { Logger, injectLambdaContext } from "@aws-lambda-powertools/logger";
import middy from "@middy/core";

const logger = new Logger({ serviceName: "someName" });

export const handler = middy(async () => {
  logger.info("test", { details: "foo" });

  try {
    throw new Error("error");
  } catch (err) {
    logger.error("some error", err as Error);
  }
}).use(injectLambdaContext(logger));

Then run the file with export POWERTOOLS_DEV=true && node lib/test.mjs, below the indented output:

Details

{
    "cold_start": true,
    "function_memory_size": null,
    "level": "INFO",
    "message": "test",
    "service": "someName",
    "timestamp": "2022-11-07T11:03:23.627Z",
    "details": "foo"
}
{
    "cold_start": true,
    "function_memory_size": null,
    "level": "ERROR",
    "message": "some error",
    "service": "someName",
    "timestamp": "2022-11-07T11:03:23.629Z",
    "error": {
        "name": "Error",
        "location": "file:///Users/aamorosi/Codes/powertools-playground/node_modules/@middy/core/index.js:85",
        "message": "error",
        "stack": "Error: error\n    at file:///Users/aamorosi/Codes/powertools-playground/lib/test.mjs:10:11\n    at runRequest (file:///Users/aamorosi/Codes/powertools-playground/node_modules/@middy/core/index.js:85:17)"
    }
}

[dreamorosi]

Hi @shdq I'm ready to approve the PR, however I have left one minor comment in the docs section. Apologies for not flagging this earlier, I noticed how it reads only after rendering the docs locally.

[shdq]

Hey @dreamorosi! Thank you for the fast feedback loop and help with this PR. I enjoyed working on it. If you need help with other issues or maybe have a particular one in mind – let me know.

[flochaz]

e2e are failing :/ https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3418100576/jobs/5690304692

[dreamorosi]

e2e are failing :/ https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3418100576/jobs/5690304692

We need to take a look at them, they're failing in Metrics which were not changed in this PR

[dreamorosi]

We have a PR open #1146 that should fix the issue highlighted by the e2e tests.

[dreamorosi]

Hi @shdq, after discussing this with the other maintainers I think we should tweak the function that resolves the value of the POWERTOOLS_DEV function so that it's closer to the implementation that the Python's version of the library has:

value = value.lower()
if value in ("1", "y", "yes", "t", "true", "on"):
    return True
if value in ("0", "n", "no", "f", "false", "off"):
    return False

Basically we should accept more values for true & check other values for false.

Also, we should update the value in the docs to say false, I have opened a PR in the Python's repo so that it's also changed to false.

Thanks for your understanding!

[dreamorosi]

See comment above about default value of POWERTOOLS_DEV and value resolution.

[ijemmy]

@shdq Sorry about this last minute after you have addressed everything.

I was the one who raised this discrepancy to Python team. It sounds wrong to me that POWERTOOLS_DEV use 0 instead of false. This could lead to configuration error for some users who aren't familiar with truthy evaluation. So we decided to change this.

I am happy to make an adjustment + add test cases myself if you don't have time for this. Just let me know.

[shdq]

Hello, @ijemmy I'll make the changes!

I'm actually looking at the python implementation of the function that resolves the value and in the situation, when the env var value is "somethingsilly" it returns None. They throw an error for the wrong values.

Here we have tests for such values.

@dreamorosi @ijemmy what do you think if I implement it as a switch statement with default: false ?

[shdq]

Another question:

There is the same check for the POWERTOOLS_LOGGER_LOG_EVENT variable. Should it also be truthy? In the docs, it says Whether to log or not the incoming event when using the decorator or middleware. Supported values are: true, or false, disabled by default. But it also checks for 1.

And there was a function isValueTrue, that was removed after this refactoring. What do you think, maybe it makes sense to make a common function for checking for true and use it for both variables? Or across all packages. How it is checking in Tracer and Metrics?

[dreamorosi]

There is the same check for the POWERTOOLS_LOGGER_LOG_EVENT variable. Should it also be truthy? In the docs, it says Whether to log or not the incoming event when using the decorator or middleware. Supported values are: true, or false, disabled by default. But it also checks for 1.

Possibly yes, since this new function is going to be implemented (at least for now) in packages/logger/src/config/EnvironmentVariablesService.ts it makes sense to do the same check there.

And there was a function isValueTrue, that was removed after this refactoring.

I opted to remove that function because at the time that was the only usage.

What do you think, maybe it makes sense to make a common function for checking for true and use it for both variables? Or across all packages. How it is checking in Tracer and Metrics?

At the moment they're not really checking for truth values but exactly and only true, however I agree that there's room for improvement and extend the same checks to these other utilities.

For now I would keep the change scoped to this PR. Later if you want, in a separate PR after this one, we could move up the logic in the packages/commons as well as potentially address #165

Let's discuss this again once this is merged.

[shdq]

@dreamorosi sorry for being dense, but I want to make sure that I got it right. I need to implement a function isValueTrue in packages/logger/src/config/EnvironmentVariablesService.ts and apply it to both POWERTOOLS_DEV and POWERTOOLS_LOGGER_LOG_EVENT checks. Correct?

Am I right, that we treat any other than "1", "y", "yes", "t", "true", "on" values as false? So it is not needed to explicitly check for falsy values.

Speaking of implementation details, I have two options:

switch (value) {
  case '1':
  case 'y':
  case 'yes':
  case 't':
  case 'true':
  case 'on':
    return true;
  default:
    return false;
}

Another option is to put it in an array of truthy values:

const truthyValues: string[] = ["1", "y", "yes", "t", "true", "on"];
return truthyValues.includes(value); // possible to use indexOf(), but it doesn't matter since it is going to be transpilled.

I like it, it is short and concise, but it uses an array. What do you think?

Thank you for your time and help!

[shdq]

Oops. I synced my fork and branch and I think I pushed it wrong.

[dreamorosi]

sorry for being dense,

Not at all, I appreciate the questions 😄

but I want to make sure that I got it right. I need to implement a function isValueTrue in packages/logger/src/config/EnvironmentVariablesService.ts and apply it to both POWERTOOLS_DEV and POWERTOOLS_LOGGER_LOG_EVENT checks. Correct?

Yes, that's correct.

Am I right, that we treat any other than "1", "y", "yes", "t", "true", "on" values as false? So it is not needed to explicitly check for falsy values.

I have to admit that I'm on the fence about this, but ultimately yes, anything that it's not in that list of truth-y values can be considered as false.

If we look at the Python's implementation they actually throw an error if the value is not in the list of falsy values, but I don't think we should do that at this stage and that we should instead default to false.

Speaking of implementation details, I have two options:

Personally I like the Array.includes option, that is how I would write it if I were to, and the switch would be my least favourite because of verbosity, but it's not a strong preference.

[shdq]

I hope I didn't mess up with the previous push. And I can't find the commit to the docs with the default false value.

Should we add tests for isValueTrue?

If any other changes are needed I'm happy to make them!

[dreamorosi]

I hope I didn't mess up with the previous push. And I can't find the commit to the docs with the default false value.

Don't worry, it happens! As far as I can see the diff looks okay and anyway we'll squash before merging so the single commits don't matter as much.

Should we add tests for isValueTrue?

Yes please, would be great to have them so we can detect regressions in the future. Maybe you could reduce the ones for getDevMode and convert some of them to test isValueTrue instead.

For getDevMode I would test only true, false, not set, and somethingsilly. The others would go to isValueTrue. For the latter I don't think there's no need to test every single combination: i.e. it's okay to test TRUE and false without the need of having to write tests for TRUE, true, FALSE, and False - we can assume that the String.toLowerCase() from the standard lib works.

Just make sure each string (case-insensitive) is represented once to test we support it.

If any other changes are needed I'm happy to make them!

As far as I can see, and from my side, that would be all 🙂

[dreamorosi]

Neat, I like it

[dreamorosi]

Looks good to me, thank you for the attention to details 💯

[dreamorosi]

E2E tests are passing: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3437044442