Skip to content
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

feat(metrics): introduce MetricsUtils.withMetricsLogger utility #1000

Merged
merged 6 commits into from
Feb 16, 2023

Conversation

humanzz
Copy link
Contributor

@humanzz humanzz commented Nov 26, 2022

closes #999

Issue #, if available: #999

Description of changes:

introduce new MetricsUtils.withMetricsLogger utility method to enable higher level of customization to the metrics logger and eliminate the need to know the metric/configuration when calling the method and leave that instead to the Consumer<MetricsLogger>

The main use case my team has

  1. We're emitting a number of metrics - ~10 metrics - that all have the same config (e.g. dimensions) that's meant to be different from MetricsUtils.metricsLogger() i.e. we need a new metrics logger instance
  2. The names of those metrics are generated dynamically - think loops, appending different constants - to generate those names

So, ideally, we create a new instance of a metrics logger, and then rely on to emit those 10 different metrics.

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.

@humanzz humanzz changed the title feat(metrics): introduce MetricUtils.withMetric utility feat(metrics): introduce MetricsUtils.withMetric utility Nov 26, 2022
@msailes msailes self-requested a review January 13, 2023 15:39
msailes
msailes previously approved these changes Jan 13, 2023
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.

LGTM

msailes
msailes previously approved these changes Feb 15, 2023
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.

LGTM

@scottgerring scottgerring self-requested a review February 15, 2023 14:55
Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

Looks good to me @humanzz - I have one question on the changed body of the try / finally inline

@kozub kozub self-requested a review February 15, 2023 15:11
@kozub
Copy link
Contributor

kozub commented Feb 15, 2023

It looks also good for me.

The only concern that I have is about naming. Right now we have two methods: withSingleMetric() and withMetric() - without the context I would have no idea what is the difference. Not sure if .withCustomMetric() or .withConfigurableMetric() sounds better. What do you thing? I don't have strong opinion what will be more natural for end user.

@humanzz
Copy link
Contributor Author

humanzz commented Feb 15, 2023

@kozub

It looks also good for me.

The only concern that I have is about naming. Right now we have two methods: withSingleMetric() and withMetric() - without the context I would have no idea what is the difference. Not sure if .withCustomMetric() or .withConfigurableMetric() sounds better. What do you thing? I don't have strong opinion what will be more natural for end user.

The names are also throwing me off a bit tbh as well. I am between

  • singular withMetric
  • plural withMetrics
  • withMetricsLogger to really be super explicit, which I think is more in line with MetricsUtils.metricsLogger() and is actually more telling about what it provides.

I'm more leaning towards the last one now that I think about it and will make a change.

Let me know what you think

@kozub
Copy link
Contributor

kozub commented Feb 15, 2023

Hey @humanzz

I think that .withMetricsLogger() sound more natural than previous version. Thanks for that change.
Idea of having .withMetric() and .withMetrics() is also interesting, but unfortunately it introduces some breaking changes so we won't be able to release it so quickly.

scottgerring
scottgerring previously approved these changes Feb 15, 2023
@scottgerring scottgerring requested review from scottgerring and removed request for scottgerring February 15, 2023 16:17
@humanzz humanzz changed the title feat(metrics): introduce MetricsUtils.withMetric utility feat(metrics): introduce MetricsUtils.withMetricsLogger utility Feb 15, 2023
@humanzz
Copy link
Contributor Author

humanzz commented Feb 15, 2023

@kozub @scottgerring apologies folks. Had to make an additional commit to fix a documentation typo

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.

Approved

@msailes msailes merged commit 7dfe9c3 into aws-powertools:master Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

metrics: Expose a MetricsUtils.withSingleMetric overload that doesn't require metric name/value/unit
4 participants