-
Notifications
You must be signed in to change notification settings - Fork 90
feat: single metric utility method to pick default namespace #305
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
Conversation
* @param unit the unit type of the metric | ||
* @param logger the MetricsLogger | ||
*/ | ||
public static void withSingleMetricOnDefaultNameSpace(final String name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Really appreciate that.
I'd say that maybe we can expand this to also set the AwsRequestId
property. Probably we should do the same for withSingleMetric
.
If we do the AwsRequestId
part that will ensure that all metrics metrcisLogger()
and withSingleMetric
methods from MetricsUtils
have the request Id and the traceId which ensures good observability and tracing
I know you've taken a very explicit approach in naming the function, but I think the OnDefaultNameSpace
suffix can be dropped in favour of documentation calling out the usage of the default namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of like the idea here. Appreciate the feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion incorporated. Now it will capture request id, trace id whenever possible as property
7451546
to
10d7ff8
Compare
...rc/test/java/software/amazon/lambda/powertools/metrics/internal/LambdaMetricsAspectTest.java
Show resolved
Hide resolved
@@ -16,6 +18,10 @@ public Object handleRequest(Object input, Context context) { | |||
MetricsLogger metricsLogger = metricsLogger(); | |||
metricsLogger.putMetric("Metric1", 1, Unit.BYTES); | |||
|
|||
|
|||
withSingleMetric("Metric2", 1, Unit.COUNT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great improvement from some great feedback.
99% of customers will be happy to see this opinionated action taken.
Great stuff!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
f8c0dc1
to
b02cc6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
**Issue #297
Description of changes:
Expose another helper utility method to pick default namespace from top level annotation or env var
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.