feat(v2): upgrade embedded-metrics to v4 #1405
Conversation
jeromevdl
changed the title
jeromevdl
changed the title
jeromevdl
force-pushed
the
feat/metrics-v4
branch
from
765f1ea to
e4317db
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@scottgerring I'm not happy with this, have a look at the examples, the experience is really bad now with these checked exceptions. We encapsulate some of them, should we go further? Review the module completely? |
|
@jeromevdl I linked this to #1041 - do we cover off the whole thing here (high-res metrics), or just the upgrade? |
|
I didn't check high-res, it's just an upgrade (don't know if high res is handled directly or if we need to do something). |
Should we look at high-res as part of this, or unlink it from the underlying issue and deal with that separately? It's one of the things on the V2 roadmap. |
high res is provided with the new version, we have nothing to do on our side. |
| @@ -44,7 +44,7 @@ | |||
| * Handler for requests to Lambda function. | |||
| */ | |||
| public class App implements RequestHandler<APIGatewayProxyRequestEvent, APIGatewayProxyResponseEvent> { | |||
| private final static Logger log = LogManager.getLogger(App.class); | |||
| private static final Logger log = LogManager.getLogger(App.class); | |||
There was a problem hiding this comment.
What's up with all the whitespace changes?
|
|
||
| package software.amazon.lambda.powertools.metrics.exception; | ||
|
|
||
| public class InvalidMetricDimensionException extends RuntimeException { |
There was a problem hiding this comment.
I gather we are using these to wrap the checked exceptions with RuntimeExceptions while retaining something catchable.
Worth explaining that and why we're doing it on the javadoc ?
| } catch (InvalidNamespaceException e) { | ||
| throw new InvalidMetricNamespaceException(e); | ||
| } catch (InvalidMetricException e) { | ||
| throw new software.amazon.lambda.powertools.metrics.exception.InvalidMetricException(e); |
There was a problem hiding this comment.
It's a bit confusing that sometimes the wrapping RuntimeException has the same name and only a different NS and sometimes not
| public class InvalidMetricNamespaceException extends RuntimeException { | ||
| private static final long serialVersionUID = -8758457068851174063L; | ||
| public static final String POWERTOOLS_METRICS_NAMESPACE_REQUIRED = | ||
| "A valid namespace is required, either pass it to the @Metrics annotation or set the environment variable POWERTOOLS_METRICS_NAMESPACE"; |
There was a problem hiding this comment.
It's not clear to me why we have custom messages inline in this one but not in e.g. InvalidMetricException
|
|
||
| context.setDimensions(defaultDimensions); | ||
|
|
||
| f.set(metricsLogger(), context); | ||
| } catch (NoSuchFieldException | IllegalAccessException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (InvalidDimensionException e) { | ||
| throw new InvalidMetricNamespaceException(e); |
There was a problem hiding this comment.
InvalidDimensionException -> InvalidMetricNamespaceException?
| try { | ||
| metricsLogger.putMetric("ColdStart", 1, Unit.COUNT); | ||
| } catch (InvalidMetricException e) { | ||
| // should not occur |
There was a problem hiding this comment.
Throw or error or something then? Empty catch blocks should at least set off a linter!
| @@ -222,8 +222,12 @@ You can create metrics using `putMetric`, and manually create dimensions for all | |||
| @Override | |||
| @Metrics(namespace = "ExampleApplication", service = "booking") | |||
| public Object handleRequest(Object input, Context context) { | |||
| metricsLogger.putDimensions(DimensionSet.of("environment", "prod")); | |||
| metricsLogger.putMetric("SuccessfulBooking", 1, Unit.COUNT); | |||
| try { | |||
There was a problem hiding this comment.
Worth documenting the exception transformation here, probably? I'm also wondering if we should reach out to the SDK team to understand their thinking for the checked exceptions here; maybe we are missing something.
jeromevdl
changed the title
|
We need to redo this from the released version of the library without the checked exceptions we have extensively worked around here |
Issue #, if available: #1041
Description of changes:
Upgrade to latest version of embedded-metrics
Checklist
Breaking change checklist
Checked exception instead of unchecked exception. I feel the new dev experience is shitty with these exceptions. Is there anything we can do?
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.