Comments (21)
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
- Emit both and mark something as deprecated for a few versions
- 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.
- 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.
- 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. :)
- 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
- All metrics from
MetricsUtils
should have properties for requestId and traceId. This ensures good observability and tracing - 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 theMetricsUtils
if some users want to use this.I think the 2 most important things on my mind right now are
- All metrics from
MetricsUtils
should have properties for requestId and traceId. This ensures good observability and tracing- 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
- 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
- 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.
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
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
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.
- I like the idea that now there's a way to set a default for the dimensions from
@Metrics
andwithSingleMetric
- 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 - Instead of
MetricsUtils.defaultDimensionSet(DimensionSet dimensionSet)
, aMetricsUtils.defaultDimensions(DimensionSet... dimensionSets)
method would address both problems above
a. CallingMetricsUtils.defaultDimensions()
(no arguments) should set the default dimensions to no dimensions
b. CallingMetricsUtils.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.
Related Issues (20)
- RFC: Remove support for Java 8 in V2 HOT 7
- Feature enhancement (Logger): automatically intercept Runtime exceptions, and log information HOT 7
- Log level not restored at the end of execution HOT 2
- Support batch secrets retrieval in Parameters module HOT 6
- Maintenance: update jackson and aws-xray-recorder dependencies together HOT 4
- RFC: handle batch messages in parallel in batch module HOT 13
- v2: Example logging configuration needs to be updated HOT 3
- v2 - Params builders do not supply default TransformationManager HOT 1
- bug - v2 - end-to-end tests broken
- AbstractCustomResourceHandler - Unable to send response HOT 2
- LambdaEcsLayout seems to incorrectly serialize the service field HOT 7
- Add support for CRaC
- RFC: support for multi-endpoint HTTP handler Lambda functions HOT 9
- Docs: Add Powertools for AWS Lambda workshop to documentation
- Propagation of request information across thread boundaries HOT 2
- Propagate traces and log context to non-main threads HOT 4
- AppConfig "empty" response handling HOT 4
- Error upgrading aws-java-sdk-v2 monrepo HOT 7
- [Support Powertools for AWS Lambda (Java)]: <your organization name> HOT 1
- RFC: Support Micronaut and Spring Cloud Function (and more?) HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from powertools-lambda-java.