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.

Sign up for GitHub

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): increase maximum dimensions to 29 #1072

Merged
merged 2 commits into from
Aug 23, 2022
Merged

feat(metrics): increase maximum dimensions to 29 #1072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27b0e87
improv(metrics): Add support to 30 dimensions
sthuber90 Aug 23, 2022
9ff1f5c
docs(metrics): update max dimensions
sthuber90 Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/core/metrics.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ If you do not use the middleware or decorator, you have to flush your metrics ma
!!! warning "Metric validation"
If metrics are provided, and any of the following criteria are not met, a **`RangeError`** exception will be thrown:

* Maximum of 9 dimensions
* Maximum of 29 dimensions
* Namespace is set only once (or none)
* Metric units must be [supported by CloudWatch](https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_MetricDatum.html)

Expand Down
2 changes: 1 addition & 1 deletion packages/metrics/src/Metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
} from './types';

const MAX_METRICS_SIZE = 100;
const MAX_DIMENSION_COUNT = 9;
const MAX_DIMENSION_COUNT = 29;
const DEFAULT_NAMESPACE = 'default_namespace';

/**
Expand Down
5 changes: 3 additions & 2 deletions packages/metrics/tests/unit/Metrics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { Metrics, MetricUnits } from '../../src/';
import { populateEnvironmentVariables } from '../helpers';

const MAX_METRICS_SIZE = 100;
const MAX_DIMENSION_COUNT = 9;
const MAX_DIMENSION_COUNT = 29;
const DEFAULT_NAMESPACE = 'default_namespace';

const consoleSpy = jest.spyOn(console, 'log').mockImplementation();
Expand Down Expand Up @@ -371,7 +371,7 @@ describe('Class: Metrics', () => {
}
});

test('Error should be thrown on empty metrics when throwOnEmptyMetrics() is callse', async () => {
test('Error should be thrown on empty metrics when throwOnEmptyMetrics() is called', async () => {
expect.assertions(1);

const metrics = new Metrics({ namespace: 'test' });
Expand Down Expand Up @@ -459,6 +459,7 @@ describe('Class: Metrics', () => {
});

test('Should throw an error if the same metric name is added again with a different unit', ()=> {
expect.assertions(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please move this assertion at the end of the test? This file specifically is a bit of an outlier, but generally speaking we try to do 1/ Setup, 2/ Action, 3/ Assess

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw that the assertion is used at the beginning in other test cases of this file, we will be changing this in the context of #163 to make them more uniform to the other two utils.

You can choose to ignore the comment or address it, it won't be a blocker for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you already said, I added it at the beginning just as it's the case in other tests. If I hadn't seen this, I would have used the fail('reason') before the catch block but that's something to be adressed in the issue that you've referenced

const metrics = new Metrics();

metrics.addMetric('test_name', MetricUnits.Count, 2);
Expand Down