[Metrics] Improvements to Powertools Metrics

[humanzz]

Is your feature request related to a problem? Please describe.
The following requests are based on a recent experience I had using Powerttools Metrics having used https://github.com/awslabs/aws-embedded-metrics-java in different projects before and the workarounds we had to implement and hoping they should be taken care of by Powertools so we can move our older projects to Powertools and have a consistent experience among the different projects.

As such, I'll mention a few points below

Describe the solution you'd like

1. Improve documentation/warnings about the fact that powertools annotations require the Lambda handler method to be called handlerRequest

   • I use CloudGuru Profiler with the setup described in https://docs.aws.amazon.com/codeguru/latest/profiler-ug/lambda-custom.html. This implementation overrides/hides handleRequest and instead exposes requestHandler method that users should override to add their logic
   • It took me a lot of time debugging/investigating this; time that could've been saved if there was better documentation or warnings about where the annotations are.
   • My workaround was to create a new BaseHandler that extends RequestHandlerWithProfiling, override handleRequest calling super.handleRequest and annotating it with the powertools annotations. Then implementing my concrete handlers as subclasses of BaseHandler

2. Metrics emitted by MetricsUtils.metricsLogger should have a property for the requestId

   • The Metric EMF entity emitted does not contain a property for the requestId. It has other useful things like traceId
   • I think the LambdaMetricsAspect can be updated to add a requestId property by default

3. Metrics emitted by MetricsUtils.metricsLogger should allow dimensions to be overriden or unset

   • By default, they get the following dimensions "Dimensions":[["LogGroup","ServiceName","ServiceType","Service"]].
   • For ServiceName and Service; that's unnecessary, I believe one to be coming from the EMF Library and the other from Powertools
     • https://github.com/awslabs/aws-embedded-metrics-java/blob/master/src/main/java/software/amazon/cloudwatchlogs/emf/logger/MetricsLogger.java#L174
     • https://github.com/awslabs/aws-lambda-powertools-java/blob/master/powertools-metrics/src/main/java/software/amazon/lambda/powertools/metrics/internal/LambdaMetricsAspect.java#L51
   • We in our existing setups with EMF, we don't find value in any of those default dimensions. We're simply able to override them using setDimensions() and giving it zero arguments if we don't need any of the default dimensions. Unfortunately this cannot be done for Powertools MetricsUtils.metricsLogger() as it fails validation if dimensions count is equal to 0

4. MetricsUtils.withSingleMetric should have improved properties and namespace configs

   • withSingleMetric requires the setting of a namespace. In most cases, one would want to use the default namespace which is not easily accessible (beyond grabbing it explicitly from the environment variables). I believe there should be a version of this method which simply reuses the namespace configured for MetricsUtils.metricsLogger
   • withSingleMetric should also add the requestId property
   • In our projects that use EMF, we have an abstraction similar to MetricsUtils. It provides the same general capabilities - a default metricsLogger that gets flushed at the end of a request + one off metrics. The one thing we have on top of that is that we're able to set some default properties that will be written to both types of metrics e.g. defaultProperties that we can append to at any time. It adds to the MetricsUtils.metricsLogger, but gets applied to the equivalent of withSingleMetrics when they're instantiated. This allows us to have a consistent set of properties common across all our EMF metrics.

Additional context
It's likely that each of topics above should be addressed in a different issue and I'm happy to break it down further if you think that's useful.

I'm also happy to attempt to contribute after discussions if needed.

[msailes]

Thanks for this great feedback!

[pankajagrawal16]

Really great feedback @humanzz , If you are happy to contribute, we will be more than happy to take above suggestions. If not, I will spend some time next week incorporating some of those.

[pankajagrawal16]

1. Improve documentation/warnings about the fact that powertools annotations require the Lambda handler method to be called handlerRequest

There is a PR up for it now which will support the profile handler as well natively now.

2. Metrics emitted by MetricsUtils.metricsLogger should have a property for the requestId

This is already merged now by @msailes . So will be made available in next release.

withSingleMetric requires the setting of a namespace. In most cases, one would want to use the default namespace which is not easily accessible (beyond grabbing it explicitly from the environment variables). I believe there should be a version of this method which simply reuses the namespace configured for MetricsUtils.metricsLogger

This is actually a good idea and I personally thought about having such support in the 1st version itself but avoided until we see value in it. But now since we have feedback from community as well, there is a PR up for it already now. :)

4. MetricsUtils.withSingleMetric should have improved properties and namespace configs
   withSingleMetric should also add the requestId property

I believe since we expose logger object as a Consumer function, clients of the API have access to it and thus can always set requestId or any other related property on for the emf logger.

In our projects that use EMF, we have an abstraction similar to MetricsUtils. It provides the same general capabilities - a default metricsLogger that gets flushed at the end of a request + one off metrics. The one thing we have on top of that is that we're able to set some default properties that will be written to both types of metrics e.g. defaultProperties that we can append to at any time. It adds to the MetricsUtils.metricsLogger, but gets applied to the equivalent of withSingleMetrics when they're instantiated. This allows us to have a consistent set of properties common across all our EMF metrics.

Same for this one, I believe if you have a common consumer function which just sets the same property you need on all your metrics, we will be able to reuse the same consumer function on all the single metrics function. And for one on the annotation level, you will need to set that on the global logger instance.

While this will be some additional code, I believe having too many high level abstraction within MetricsUtils is not very scalable and can soon start to being complex. But thats just me thinking out loud. I will be happy to hear thoughts from others as well.

[humanzz]

Thanks everyone. Really appreciate the prompt actions on addressing the feedback.

@pankajagrawal16
I've left you a comment on #305
I do get the concern around over complicating things with the last quoted paragraph. I wouldn't call this a must have, it's easily achievable on user-side by wrapping the MetricsUtils if some users want to use this.

I think the 2 most important things on my mind right now are

1. All metrics from MetricsUtils should have properties for requestId and traceId. This ensures good observability and tracing
2. All metrics from MetricsUtils should enable overriding the default dimensions to set them to no dimensions

[pankajagrawal16]

Thanks everyone. Really appreciate the prompt actions on addressing the feedback.

@pankajagrawal16
I've left you a comment on #305
I do get the concern around over complicating things with the last quoted paragraph. I wouldn't call this a must have, it's easily achievable on user-side by wrapping the MetricsUtils if some users want to use this.

I think the 2 most important things on my mind right now are

1. All metrics from MetricsUtils should have properties for requestId and traceId. This ensures good observability and tracing
2. All metrics from MetricsUtils should enable overriding the default dimensions to set them to no dimensions

I agree with the thoughts. Default dimension part is something which I will have a look at next. This has been one of items that I have wanted to fix for a while already and now that we have community feedback as well around it, I sure will.

[pankajagrawal16]

I believe most of these suggestion are incorporated except for point 3

[humanzz]

Awesome progress. Thanks a lot @msailes and @pankajagrawal16

I have a couple of comments to add

1. Re 3 (the ability to remove the default dimensions)
   • @msailes mentioned it might be a good idea to open an issue for EMF library to consider dropping those default dimensions - or simply add them as properties
   • My feeling is that this will likely take sometime. Meanwhile, the ability to override the dimensions for all MetricsUtils loggers would be really appreciated. It allows us users to be in control if we want to override it and remove dimensions
2. Regarding requestId and traceId, I think between logging and metrics, they have different field names which is not ideal because ideally I - as a user - would want to use requestId or traceId to filter for all log entries in CloudWatch Logs Insights. When the field names between logging and metrics are different this unnecessarily complicates such types of queryes
   • Metrics: https://github.com/awslabs/aws-lambda-powertools-java/blob/master/powertools-metrics/src/main/java/software/amazon/lambda/powertools/metrics/internal/LambdaMetricsAspect.java#L31
   • Logging
     • https://github.com/awslabs/aws-lambda-powertools-java/blob/master/powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/DefaultLambdaFields.java#L45
     • https://github.com/awslabs/aws-lambda-powertools-java/blob/master/powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspect.java#L91

[pankajagrawal16]

2. Regarding requestId and traceId, I think between logging and metrics, they have different field names which is not ideal because ideally I - as a user - would want to use requestId or traceId to filter for all log entries in CloudWatch Logs Insights. When the field names between logging and metrics are different this unnecessarily complicates such types of queryes

   • Metrics: https://github.com/awslabs/aws-lambda-powertools-java/blob/master/powertools-metrics/src/main/java/software/amazon/lambda/powertools/metrics/internal/LambdaMetricsAspect.java#L31

   • Logging

     • https://github.com/awslabs/aws-lambda-powertools-java/blob/master/powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/DefaultLambdaFields.java#L45
     • https://github.com/awslabs/aws-lambda-powertools-java/blob/master/powertools-logging/src/main/java/software/amazon/lambda/powertools/logging/internal/LambdaLoggingAspect.java#L91

Makes sense, Since Logging already had those fields, it will make sense to just reuse the same names for metrics module. I will fix that before new release

[pankajagrawal16]

I have a couple of comments to add

1. Re 3 (the ability to remove the default dimensions)

   • @msailes mentioned it might be a good idea to open an issue for EMF library to consider dropping those default dimensions - or simply add them as properties
   • My feeling is that this will likely take sometime. Meanwhile, the ability to override the dimensions for all MetricsUtils loggers would be really appreciated. It allows us users to be in control if we want to override it and remove dimensions

Yeah rite, thats the next thing which I plan to look at next. It might be a good idea to open a issue on EMF lib as well so that we can simplify powertools implementation once its natively available as well

[humanzz]

My personal taste would be for the new names used in metrics to be honest but it's not a big deal if we don't go with that option

[pankajagrawal16]

My personal taste would be for the new names used in metrics to be honest but it's not a big deal if we don't go with that option

Yeah, But in order to avoid this being a break change for logging clients (in the sense if they have some filters like you mentioned), it makes sense to reuse the same for metrics lib.

[humanzz]

I agree - from a compatability point of view - but I still do think the new names are slightly better so I wonder if we can either/or

1. Emit both and mark something as deprecated for a few versions
2. Have a separate issue to track these names for an upcoming major version that might introduce breaking changes

Regardless, unification is the thing we should go for.

[pankajagrawal16]

@humanzz Most of the stuff is available in 1.3.0 now. I will keep this open for fixing the last bit on default dimensions.

[humanzz]

That's great, about the dimensions @pankajagrawal16
Will you also support overriding to set no dimensions?

[pankajagrawal16]

That's great, about the dimensions @pankajagrawal16

Will you also support overriding to set no dimensions?

Not at this point. Since this is a core validation which is present in python version as well and if we allow it then we have to do it across. So best will be to open a separate RFC for it.

[heitorlessa]

Powertools Python folks here ;) -- EMF Spec originally had a dimension as a minimum otherwise metrics weren't created and failed silently --- That is not the case anymore.

As of last Friday, we've released Lambda Powertools Python 1.11 removing the validation of a minimum one dimension.

[pankajagrawal16]

Powertools Python folks here ;) -- EMF Spec originally had a dimension as a minimum otherwise metrics weren't created and failed silently --- That is not the case anymore.

As of last Friday, we've released Lambda Powertools Python 1.11 removing the validation of a minimum one dimension.

Thanks for confirming that Heitor, That means I will update it for java as well in the same PR.

[humanzz]

Happy times :-)

[pankajagrawal16]

All of the suggestions are available in 1.4.0 now

[humanzz]

Thanks a lot @msailes and @pankajagrawal16

Thanks a lot for addressing all of this. I actually have some follow up comments on MetricsUtils.defaultDimensionSet which I did not notice when the PRs were going.

1. I like the idea that now there's a way to set a default for the dimensions from @Metrics and withSingleMetric
2. However, I think there's still some room for improvement here
   a. The current interface does not permit setting no dimensions as a default
   b. The current interface does not permit setting multiple dimension sets as a default
3. Instead of MetricsUtils.defaultDimensionSet(DimensionSet dimensionSet), a MetricsUtils.defaultDimensions(DimensionSet... dimensionSets) method would address both problems above
   a. Calling MetricsUtils.defaultDimensions() (no arguments) should set the default dimensions to no dimensions
   b. Calling MetricsUtils.defaultDimensions(ds1, ds2,...) allows setting the default to have multiple dimension sets

If you agree, I think a way forwards can be to implement MetricsUtils.defaultDimensions, make MetricsUtils.defaultDimensionSet use it while marking is as deprecated for removal in a coming major version release.

What do you think?

[pankajagrawal16]

I believe I agree to allow multiple. By the way, you can just pass new DimensionSet() and that should set it with no dimensions