Mertics default dimensions #329
Conversation
|
@humanzz Have a look |
| @@ -137,7 +152,8 @@ private static MetricsLogger logger() { | |||
| MetricsContext metricsContext = new MetricsContext(); | |||
|
|
|||
| if (hasDefaultDimension()) { | |||
| metricsContext.setDefaultDimensions(defaultDimensionSet()); | |||
| metricsContext.setDefaultDimensions(new DimensionSet()); | |||
There was a problem hiding this comment.
I think this is not needed as setDimensions overrides the defaults or is there something am missing?
There was a problem hiding this comment.
That is true.
| MetricsUtils.defaultDimensionSet = dimensionSet; | ||
|
|
||
| if(dimensionSet.getDimensionKeys().size() > 0) { | ||
| MetricsUtils.defaultDimensions = new DimensionSet[]{dimensionSet}; |
There was a problem hiding this comment.
I think MetricsUtils.defaultDimensions(dimensionSet) should do the trick here
| @@ -80,7 +79,7 @@ public void metricsWithoutColdStart() { | |||
| mocked.when(() -> SystemWrapper.getenv("AWS_EMF_ENVIRONMENT")).thenReturn("Lambda"); | |||
| internalWrapper.when(() -> getenv("_X_AMZN_TRACE_ID")).thenReturn("Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8;Sampled=1\""); | |||
|
|
|||
| MetricsUtils.defaultDimensionSet(new DimensionSet()); | |||
| MetricsUtils.defaultDimensions(null); | |||
There was a problem hiding this comment.
I am not quite sure I follow what setting it to null means here? if no dimensions, shouldn't it just be MetricsUtils.defaultDimensions()? or is this because of getter/setter having the same name? If so, then the case for setting no dimensions as a default is a bit complicated UX-wise.
There was a problem hiding this comment.
Its more of a limitation of how MockedStatic works and test specific.
Ux for setting no dimension is now added as part of a test as well https://github.com/awslabs/aws-lambda-powertools-java/blob/02e43bf22fbdea163ae5663049d447d3c8c65f52/powertools-metrics/src/test/java/software/amazon/lambda/powertools/metrics/handlers/PowertoolsMetricsEnabledDefaultNoDimensionHandler.java#L16
| @@ -264,7 +263,7 @@ public void exceptionWhenNoMetricsEmitted() { | |||
| try (MockedStatic<SystemWrapper> mocked = mockStatic(SystemWrapper.class)) { | |||
| mocked.when(() -> SystemWrapper.getenv("AWS_EMF_ENVIRONMENT")).thenReturn("Lambda"); | |||
|
|
|||
| MetricsUtils.defaultDimensionSet(new DimensionSet()); | |||
| MetricsUtils.defaultDimensions((DimensionSet) null); | |||
There was a problem hiding this comment.
nit: I see other statement not having to use the type casting, is it really needed here?
There was a problem hiding this comment.
No, its not . Just a miss
| run: | | ||
| echo "GIT_PYTHON_REFRESH=quiet" |
There was a problem hiding this comment.
There was a failing build on docs and some research suggested to use it coz of some git behaviour changes
**Issue #297
Description of changes:
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.