Skip to content
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

Feature request: Option to only publish metrics when there are metrics to publish #2036

Open
1 of 2 tasks
everett1992 opened this issue Feb 7, 2024 · 5 comments · May be fixed by #3057
Open
1 of 2 tasks

Feature request: Option to only publish metrics when there are metrics to publish #2036

everett1992 opened this issue Feb 7, 2024 · 5 comments · May be fixed by #3057
Assignees
Labels
confirmed The scope is clear, ready for implementation feature-request This item refers to a feature request for an existing or new utility metrics This item relates to the Metrics Utility

Comments

@everett1992
Copy link

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.

No application metrics to publish. The cold-start metric may be published if enabled. If application metrics should never be empty, consider using `throwOnEmptyMetrics`
{"_aws":{"Timestamp":1707345436427,"CloudWatchMetrics":[{"Namespace":"ns","Dimensions":[["service"]],"Metrics":[]}]},"service":"service_undefined"}

Solution/User Experience

I see this warning was added in a previous feature #1397 so it's unlikely we would remove it. I'd be happy with a new option doNotWarnOnEmptyMetrics, or changing throwOnEmptyMetrics to accept true | false | 'some-option-to-disable-warnings'.

I'd also be happy if storedMetrics was public, or there was a public method empty(): boolean so I could write my own middleware that calls metric.empty() || metric.publishStoredMetrics().

Alternative solutions

No response

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@everett1992 everett1992 added triage This item has not been triaged by a maintainer, please wait feature-request This item refers to a feature request for an existing or new utility labels Feb 7, 2024
@dreamorosi dreamorosi added metrics This item relates to the Metrics Utility discussing The issue needs to be discussed, elaborated, or refined labels Feb 9, 2024
@dreamorosi
Copy link
Contributor

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.

@dreamorosi dreamorosi added need-more-information Requires more information before making any calls and removed triage This item has not been triaged by a maintainer, please wait labels Feb 9, 2024
@everett1992
Copy link
Author

everett1992 commented Feb 9, 2024 via email

@dreamorosi
Copy link
Contributor

dreamorosi commented Feb 15, 2024

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.

@everett1992
Copy link
Author

hasStoredMetrics(): boolean would hide implementation and expose enough information for me.

@dreamorosi dreamorosi self-assigned this Sep 12, 2024
@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined need-more-information Requires more information before making any calls labels Sep 12, 2024
@dreamorosi dreamorosi linked a pull request Sep 12, 2024 that will close this issue
@dreamorosi
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed The scope is clear, ready for implementation feature-request This item refers to a feature request for an existing or new utility metrics This item relates to the Metrics Utility
Projects
Status: Working on it
Development

Successfully merging a pull request may close this issue.

2 participants