-
Notifications
You must be signed in to change notification settings - Fork 4
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
RFC: New environment variable POWERTOOLS_DEV to ease prototyping #86
Comments
Will this be expected to not be used when running (in a sandbox/dev) account on Lambda? I might want to convenience of enabling all dev-functionality at once (logging the full event might also be one?), without it impacting how I see things in CloudWatch. |
Exactly, we won't impact the final runtime. I'll make that clearer in the RFC body.
That's exactly the main argument to use |
Updated the body of the RFC, and also posed a question if a customer accidentally deploys a function with Changes
|
I think I was thinking about it the other way. If this enables (in the future) things like more verbose logging, or logging of more things (inputs / outputs / ...), it might be nice to have it available in a Lambda Function too (if that function is in a developer's account and not getting a lot of traffic). EDIT: or to put it more clearly: I assumed |
I do see where you're coming from tho - I made the wording crispier and brought in as a what if question before we settle on this. In hindsight, I guess we can't protect customers 100% from forgetting that parameter, and putting a guard would prevent you from utilizing in non-prod env. I initially thought it'd be more useful in local executions, but I can see cases where you'd want that deployed when multiple functions are involved. On the naming itself, the reason we didn't propose |
If this will always switch indentation, I think it's fine to prevent me from using it (but maybe it should print an INFO one-liner to the logs). Setting POWERTOOLS_DEV to True and toggling indentation to be off, is the same effort as toggling individual debug settings.
Naming is always difficult :). |
Yeah, hmmm. Future wise, I can't see how we'd detect you're running on Fargate/Glue/Docker and auto-toggle that off at runtime. What if we simply log a warning ( That could solve our side on trying too hard to auto-toggle off this feature in scenarios we can't always predict, and keeping customers informed about the outcome when enabled. |
@dreamorosi @saragerion @rubenfonseca @leandrodamascena what do you think of In hindsight, whatever name we settle, we could enable another one |
From @jfuss
My only concern with an enum is that it opens the room for typos given this is an env var. We could technically have a default in the Enum ( |
Would you log it everywhere, or only in lambda? In the second case, I think the indentation would show it's not a great idea anyway. In the first case, there is a balance between annoying and helpful, but warnings about security and/or cost seem to be on the helpful side (maybe it should log outside the handler, so that environments that keep the function warm don't log on every invocation) |
The former. Like we do in Metrics today - we will log a warning (anywhere) if you try to flush metrics without a metric per se - it might be intentional, but we won't raise an Exception unless you tell us to ( |
👍🏻
If used as intended, that failure should be spotted fast ("I enabled better logging, and I'm not getting better logging"), adding a warning "DECV is not a valid value for POWERTOOLS_MODE" will also point in the right direction However, I think as long as the intent is clear, a boolean is fine too. A ℹ️ in the docs saying that it will not be very useful when running with CloudWatch might be nice though. |
Hypothetically, say we have If you use |
Yes. But having |
Here's a quick POC -- from a customer point of view, they would only use an environment variable and a string value, not in their code per se. >>> mode = PowertoolsMode("XPTO")
UserWarning: Invalid value: XPTO, defaulting to PROD mode. Valid options available: ['DEV', 'PROD', 'DEBUG']
>>> mode
<PowertoolsMode.PROD: 'PROD'> import warnings
from enum import Enum
class PowertoolsMode(Enum):
DEV = "DEV"
PROD = "PROD"
DEBUG = "DEBUG"
@classmethod
def _missing_(cls, name):
warnings.warn(f"Invalid value: {name}, defaulting to PROD mode. Valid options available: {list(cls.__members__)}")
return cls.PROD |
Thanks for creating the RFC. In my opinion the goal of this environment variable should be bringing better developer experience. This should also be reflected in the docs / messaging around it, but also in the name:
My preference (in order):
For the purpose of this RFC, I would also leave out the discussion about About the enum/ At this stage I'd be inclined to keep things simple and stick to About the open question(s):
At least at this stage, when it comes to logging, CloudWatch already indents JSON structured logs when you expand an entry in a Log group. So it's safe to say that the setting should not be used on AWS Lambda and a warning could be enough. When it comes to extra data, while I see some value in emitting a warning, I'm also not sure how the environment variable used in local dev or unit tests could end up in the env vars configuration on Lambda. Both manual deployment (via Console) or IaC require a conscious effort to set env variables. I'm really on the fence on this topic. Adding a warning seems sensible and relatively low effort so I'm not against it. |
Thank you for this great discussoin. While I understand the motivation for the change, I'm worried about I think there is room for having both environment variables:
So unless I'm missing something, I would love to separate the two variables that control separate behaviour. Yes, I can have |
Thank you @rubenfonseca @dreamorosi - that's super helpful to hear your input. After re-reading the entire RFC, I'm inclined to move forward with If either @rubenfonseca @dreamorosi @leandrodamascena @saragerion @msailes disagrees, please shout out until tomorrow. I'll create a separate PR in the Python repo to document when to use which and their effects as per RFC. Thank you also to @benbridts @jfuss |
@heitorlessa can we recap shortly in bullet points the difference between the 2 and the impact on each utility? |
Absolutely. I'll update the RFC body tomorrow too: POWERTOOLS_DEVProvides additional information when prototyping an application. Effect on utilities:
POWERTOOLS_DEBUGConfigures Lambda Powertools internal logging for debug logging for all operations. Current Python implementation following AWS Python SDK standard: https://awslabs.github.io/aws-lambda-powertools-python/latest/#debug-mode |
RFC body updated to reflect the direction we agreed. @pmarko1711 was kind enough to contribute the first implementation for Before I close as successful, does anyone know of any ready-to-use automation to convert a RFC issue into a PR as markdown? Storing these in this repo will ease discovery of cross-languages specifics. |
@heitorlessa, I saw that Python has already merged the changes related to Should |
It would break customers test if that was the case — I documented as
“future proof” and Tracer continues to work as-is (disabled when not in
Lambda)
https://awslabs.github.io/aws-lambda-powertools-python/latest/#optimizing-for-non-production-environments
…On Wed, 19 Oct 2022 at 12:26, Andrea Amorosi ***@***.***> wrote:
@heitorlessa <https://github.com/heitorlessa>, I saw that Python has
already merged the changes related to POWERTOOLS_DEV & Logger, however
I'm not clear from the RFC body on whether we should also do these changes
to Tracer.
Should POWERTOOLS_DEV=${truthy_value) disable the Tracer? I'd say yes as
I consider this an explicit disable, but wanted to make sure it's in line
with the RFC before moving forward.
—
Reply to this email directly, view it on GitHub
<#86 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBGYTWDQIF46H2WBYD3WD7EETANCNFSM6AAAAAAQXZFZTM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I'm not sure I'm following. We both agree that Tracer existing behavior shouldn't change. This includes explicitly disabling it via env variable My comment was referred to: should From the doc section you linked the answer seems to be yes. But then, maybe I'm missing how it would break existing customers since it's a new setting that doesn't change/modifies other existing ones. I'm sorry if this discussion is a bit redundant but I'm not 100% clear and I prefer to clarify |
Isn't this config akin to tying behavior to NODE_ENV (which I think most people agree is not a good idea)? E.g.: if I want pretty-print on my logs I will have to accept other behaviors that I may not want (it's three or four now, but if not careful it tends to grow over time). I am not against the convenience of having an easier configuration, but please don't take away granular control. Also, there may be a third alternative: built-in config presets (e.g.: |
Hey @heitorlessa and @dreamorosi! Do we still need to keep this issue open or can we close it as complete? |
Hey Leo, not sure about Python but in TS we only implemented this for Logger, the proposal also mentioned Event Handler (which we don't have yet) and Tracer. Depending on the status, and whether we still want to move forward with the proposal you can close it or leave it, I think. |
launched it |
Is this related to an existing feature request or issue?
Which AWS Lambda Powertools utility does this relate to?
Logger, Event Handler, and potentially others
Summary
This RFC outlines the introduction of a new environment variable
POWERTOOLS_DEV
to ease local prototyping, debugging, and why not per utility env var.Use case
Powertools optimizes certain outputs for costs (CloudWatch Logs) and to facilitate native integrations (CloudWatch Embedded Metric Format, Batch processing, etc.). It also disables certain behaviours such as tracing when running outside Lambda, so adding Powertools doesn't get in the way.
As our customer base increases, it diversifies its usage and thus lies the opportunity to adapt and improve. For example, customers are running their Lambda function locally in various forms: AWS Chalice, AWS SAM CLI, AWS Amplify CLI, Serverless framework, etc.
This increases our complexity as we try to keep track of potential sources of local emulation and their many environment variables. Notwithstanding customers who may also run Lambda Powertools in non-Lambda environments such as Glue jobs, Fargate, whether we provide support or not.
Proposal
Introduce
POWERTOOLS_DEV
boolean environment variable for the following effect:*
). This will deprecatePOWERTOOLS_EVENT_HANDLER_DEBUG
that achieves the same effect in Event Handler Debug ModeThis change future-proof additional convenience for customers and lower cognitive load on needing multiple env vars to the same end. We will create a new section in the documentation to keep track of behaviours this env var will enable.
Warning increased cost in log ingestion
We anticipate customers wanting to use
POWERTOOLS_DEV
in non-production AWS accounts. This might also occur unwillingly. Regardless of the intention, we should log a warning message to signal this feature is in use. This should help prevent an increase in log ingestion cost in production.Out of scope
Change Tracer auto-disable behaviour in non-Lambda environments. This will cause a breaking change in customers unit tests, which is a great convenient Powertools feature.
We can revisit it when (if) we officially support compute beyond Lambda.
Potential challenges
An open question we have is: What if a customer intentionally deploys with
POWERTOOLS_DEV=true
? See Warning increased cost in log ingestionShould we allow it or safely ignore it (e.g.,
POWERTOOLS_DEV && not TASK_ROOT
)? Tracer onlyDependencies and Integrations
None
Alternative solutions
Introduce an environment variable per utility when the need arises, for example
POWERTOOLS_LOGGER_INDENTED_LOGS
,POWERTOOLS_EVENT_HANDLER_INDENTED_LOGS
.Acknowledgment
The text was updated successfully, but these errors were encountered: