Skip to content

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

Merged
merged 4 commits into from
Mar 1, 2021

Conversation

pankajagrawal16
Copy link
Contributor

@pankajagrawal16 pankajagrawal16 commented Feb 26, 2021

**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 #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* @param unit the unit type of the metric
* @param logger the MetricsLogger
*/
public static void withSingleMetricOnDefaultNameSpace(final String name,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@pankajagrawal16 pankajagrawal16 force-pushed the feat-metric-utility-method branch from 7451546 to 10d7ff8 Compare February 27, 2021 13:11
@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

msailes
msailes previously approved these changes Mar 1, 2021
Copy link
Contributor

@msailes msailes left a 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!

msailes
msailes previously approved these changes Mar 1, 2021
Copy link
Contributor

@msailes msailes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Copy link
Contributor

@msailes msailes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pankajagrawal16 pankajagrawal16 merged commit a1cfe72 into master Mar 1, 2021
@pankajagrawal16 pankajagrawal16 deleted the feat-metric-utility-method branch March 1, 2021 13:04
This was referenced Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants