Open everett1992 opened 8 months ago
Hi @everett1992, thank you for opening this issue and proposing the feature.
We added that warning because both EMF and Otel support the concept of high cardinality data alongside metrics. In layman terms that means you can add metadata close to your metrics to better frame them in the context of your business. This context is added throughout different operations, and can later be used to follow a transaction end-to-end.
In practical terms, customers can call addMetadata
to add all sorts of tings, and in some cases make use of sparse metrics - metrics that are created conditionally instead of having a value of 0
. Given the conditional nature of these sparse metrics customers might be adding important data as metadata but then lose it because it was not published as a metric due to the conditional nature of sparse metrics.
For this reason we decided to add a warning log for when the publishStoredMetrics
function is called but there's no metric to publish.
Silencing these logs makes sense both from a cost perspective and an operational one, especially if you know for sure that some invocations might not have metrics to publish by design.
With that said, I'm kind of reticent to add a special flag for disabling warnings and I think, if anything, that we should invest efforts to leverage existing logging and warning mechanisms so that customers can opt-out through those.
This rationale comes from looking at what other languages are doing in this space. Taking Powertools Python as an example, they log this warning using the warning
standard module. This gives customers a way to suppress warnings without having to change their code.
In this version of Powertools we instead simply emit the warning using console.log
, which makes it impossible for you to opt-out and suppress the warning. Node.js has a similar [emitWarning
method](https://nodejs.org/api/process.html#processemitwarningwarning-options), however my understanding is that it emits an Error
, so we should investigate how this behaves in the Lambda node.js managed runtime.
Other alternatives to this would be to see if we can recommend customers to enable the new Lambda Advanced Logging Controls to filter this out, or explore a deeper integration with Powertools Logger on our side, which would allow you to choose your log level.
I don't have an immediate concrete proposal for this, but it's definitely something I'd like to explore further since we have warnings and debug logs of this kind in other places of the codebase.
Thanks for the detailed response. What do you think of my proposal to make stored metric, or a count of stored metrics public. With that I could conditionally call publish stored metrics.
On Fri, Feb 9, 2024, 8:37 AM Andrea Amorosi @.***> wrote:
Hi @everett1992 https://github.com/everett1992, thank you for opening this issue and proposing the feature.
We added that warning because both EMF and Otel support the concept of high cardinality data alongside metrics. In layman terms that means you can add metadata close to your metrics to better frame them in the context of your business. This context is added throughout different operations, and can later be used to follow a transaction end-to-end.
In practical terms, customers can call addMetadata to add all sorts of tings, and in some cases make use of sparse metrics - metrics that are created conditionally instead of having a value of 0. Given the conditional nature of these sparse metrics customers might be adding important data as metadata but then lose it because it was not published as a metric due to the conditional nature of sparse metrics.
For this reason we decided to add a warning log for when the publishStoredMetrics function is called but there's no metric to publish.
Silencing these logs makes sense both from a cost perspective and an operational one, especially if you know for sure that some invocations might not have metrics to publish by design.
With that said, I'm kind of reticent to add a special flag for disabling warnings and I think, if anything, that we should invest efforts to leverage existing logging and warning mechanisms so that customers can opt-out through those.
This rationale comes from looking at what other languages are doing in this space. Taking Powertools Python as an example, they log this warning using the warning standard module. This gives customers a way to suppress warnings without having to change their code.
In this version of Powertools we instead simply emit the warning using console.log, which makes it impossible for you to opt-out and suppress the warning. Node.js has a similar emitWarning method, however my understanding is that it emits an Error, so we should investigate how this behaves in the Lambda node.js managed runtime.
Other alternatives to this would be to see if we can recommend customers to enable the new Lambda Advanced Logging Controls https://aws.amazon.com/blogs/compute/introducing-advanced-logging-controls-for-aws-lambda-functions/ to filter this out, or explore a deeper integration with Powertools Logger on our side, which would allow you to choose your log level.
I don't have an immediate concrete proposal for this, but it's definitely something I'd like to explore further since we have warnings and debug logs of this kind in other places of the codebase.
— Reply to this email directly, view it on GitHub https://github.com/aws-powertools/powertools-lambda-typescript/issues/2036#issuecomment-1936241885, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAE6WM2G5J6WRQAPQVKKE6DYSZGGLAVCNFSM6AAAAABC6UTDVGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZWGI2DCOBYGU . You are receiving this because you were mentioned.Message ID: @.*** .com>
I'm not particularly inclined in making it public
since we consider that field an implementation detail. Exposing it as public comes with the expectation that changes to it are to be considered breaking changes and require a major version.
Given it's a fairly low-level field in the implementation I think that would be limiting on our part, and unnecessarily risky on the customer side.
An alternative would be to switch storedMetrics
from private
to protected
. This way you could extend the class and access it. In this case the field could change or disappear in any minor or patch version and without notice, so you'd assume the risk of building on a non public API.
hasStoredMetrics(): boolean
would hide implementation and expose enough information for me.
hasStoredMetrics(): boolean
would hide implementation and expose enough information for me.
I have opened a PR that adds this public method as well as add the ability to pass a custom logger object to the utility.
With this custom logger, you will be able to control the logging behavior of warnings by setting a log level or suppress them entirely.
Use case
I'd like to use the
logMetrics
middleware without seeing a warning that I'm not publishing metrics, or writing an empty metrics object to EMF.I have a Lambda that may emit 0 metrics. I know this is a possibility and don't consider it an issue. But I use the
logMetrics
middleware which 1. requires me to create any metrics I may emit at the start of my function, and 2. always calls publishStoredMetrics which logs a warning and emits empty metrics.Solution/User Experience
I see this warning was added in a previous feature https://github.com/aws-powertools/powertools-lambda-typescript/pull/1397 so it's unlikely we would remove it. I'd be happy with a new option
doNotWarnOnEmptyMetrics
, or changingthrowOnEmptyMetrics
to accepttrue | false | 'some-option-to-disable-warnings'
.I'd also be happy if
storedMetrics
was public, or there was a public methodempty(): boolean
so I could write my own middleware that callsmetric.empty() || metric.publishStoredMetrics()
.Alternative solutions
No response
Acknowledgment
Future readers
Please react with 👍 and your use case to help us understand customer demand.