docs(logger): remove logEvent from required settings #1821
Conversation
boring-cyborg
bot
added
the
documentation
| @@ -50,8 +50,6 @@ These settings will be used across all logs emitted: | |||
| | ---------------------- | ---------------------------------------------------------------------------------------------------------------- | ------------------------------- | ------------------- | ------------------------------------------------------ | ------------------- | --------------------- | | |||
| | **Service name** | Sets the name of service of which the Lambda function is part of, that will be present across all log statements | `POWERTOOLS_SERVICE_NAME` | `service_undefined` | Any string | `serverlessAirline` | `serviceName` | | |||
| | **Logging level** | Sets how verbose Logger should be, from the most verbose to the least verbose (no logs) | `POWERTOOLS_LOG_LEVEL` | `INFO` | `DEBUG`, `INFO`, `WARN`, `ERROR`, `CRITICAL`, `SILENT` | `ERROR` | `logLevel` | | |||
| | **Log incoming event** | Whether to log or not the incoming event when using the decorator or middleware | `POWERTOOLS_LOGGER_LOG_EVENT` | `false` | `true`, `false` | `false` | `logEvent` | | |||
| | **Debug log sampling** | Probability that a Lambda invocation will print all the log items regardless of the log level setting | `POWERTOOLS_LOGGER_SAMPLE_RATE` | `0` | `0.0` to `1` | `0.5` | `sampleRateValue` | | |||
There was a problem hiding this comment.
Rather than removing the sampleRateValue from here I would instead change the wording above to remove the "required" mention entirely.
My reasoning here is that none of the settings is really required since they all have defaults and they are all typed as such.
So to sum up I'd change the first sentence from what it is now to something along the lines of: The logger accepts three settings. or something similar.
There was a problem hiding this comment.
My goal was to align it with python docs and have a dedicated section for env variables.
I think the confusion comes from the phrase 'or pass them in the constructor' and because we mix environment variables and constructor settings, which overlap with exceptions. So if we keep three ENVs that are also part of the constructor people might assume that the other constructor values are missing from the docs.
We could add additional deep link to API docs to the constructor. What do you think?
There was a problem hiding this comment.
I think having a section to the env variables or linking to the existing one in the main page is great.
I was mainly arguing against the "required" wording which is really not true, and at the same time wanting to have a place to get started quickly.
As a user I want to read at the very top of the page how to get started using the library either because I don't know or remember it. Having all the applicable settings surfaced early is useful and we have evidence that customers are doing this (we had multiple reports about logEvent being out of place here).
I'm in favor of deep linking the API docs and making the wording clearer if needed, but I wouldn't remove the parameter from here.
docs/core/logger.md
Outdated
| @@ -174,6 +172,9 @@ When debugging in non-production environments, you can instruct Logger to log th | |||
|
|
|||
| 1. Binding your handler method allows your handler to access `this` within the class methods. | |||
|
|
|||
| Logging incoming events only works with middy or decorator by using `injectLambdaContext`. | |||
| Only setting `POWEETOOLS_LOGGER_LOG_EVENT` to `true` will not log the incoming event. | |||
There was a problem hiding this comment.
There's a typo in the environment variable name
docs/core/logger.md
Outdated
| @@ -174,6 +172,9 @@ When debugging in non-production environments, you can instruct Logger to log th | |||
|
|
|||
| 1. Binding your handler method allows your handler to access `this` within the class methods. | |||
|
|
|||
| Logging incoming events only works with middy or decorator by using `injectLambdaContext`. | |||
There was a problem hiding this comment.
I think this first sentence is somewhat redundant as it rephrases the one above almost entirely (you can instruct Logger to log the incoming event with the middleware/decorator).
I think most of the confusion we have seen reports of is related to the fact that just enabling the env variable is not enough, which is addressed by the next sentence you've added.
Maybe we could just rephrase the initial sentence to be more specific rather than adding another one. What do you think?
docs/core/logger.md
Outdated
| @@ -389,6 +390,18 @@ The error will be logged with default key name `error`, but you can also pass yo | |||
| !!! tip "Logging errors and log level" | |||
| You can also log errors using the `warn`, `info`, and `debug` methods. Be aware of the log level though, you might miss those errors when analyzing the log later depending on the log level configuration. | |||
|
|
|||
| ### Environment variables | |||
There was a problem hiding this comment.
We have this table in the main page (here) where we keep track of all env variables, including the ones below.
Should we just link that one? I'm not sure, what do you think?
There was a problem hiding this comment.
Yes, let's keep it one place.
|
* docs: removed required settings and added section for envs * fix typo * remove env section in logs, adjust woring and link to api and index page * clarified wording on logEvent
Description of your changes
This PR removes the
logEventandsampleRateValuevalues from the sections of required parameters. I have introduced a new section Environment variables to collect all the option in one place. In addition, I added a small clarification thatPOWERTOOLS_LOGGER_LOG_EVENTonly works withinjectLambdaContextto avoid misunderstanding.Related issues, RFCs
Issue number: closes #1814
Checklist
Breaking change checklist
Is it a breaking change?: NO
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.