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.

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.

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

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

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!

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:

{
    "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)"
    }
}

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.

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

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

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

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!

@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.

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 ?

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?

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.

@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!

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

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.

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!

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 🙂

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