[Metrics] Improvements to Powertools Metrics about powertools-lambda-java HOT 21 CLOSED

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.

from powertools-lambda-java.

@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.

from powertools-lambda-java.

Thanks for this great feedback!

from powertools-lambda-java.

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.

from powertools-lambda-java.

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.

from powertools-lambda-java.

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

from powertools-lambda-java.

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.

from powertools-lambda-java.

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

from powertools-lambda-java.

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

from powertools-lambda-java.

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

from powertools-lambda-java.

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

from powertools-lambda-java.

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

from powertools-lambda-java.

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.

from powertools-lambda-java.

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

from powertools-lambda-java.

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.

from powertools-lambda-java.

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.

from powertools-lambda-java.

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.

from powertools-lambda-java.

Happy times :-)

from powertools-lambda-java.

All of the suggestions are available in 1.4.0 now

from powertools-lambda-java.

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?

from powertools-lambda-java.

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

from powertools-lambda-java.