Skip to content

Explicitly type convert metric values to Number(). #107

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

Closed
wants to merge 1 commit into from
Closed

Explicitly type convert metric values to Number(). #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/logger/MetricsContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,9 @@ export class MetricsContext {
public putMetric(key: string, value: number, unit?: Unit | string): void {
const currentMetric = this.metrics.get(key);
if (currentMetric) {
currentMetric.addValue(value);
currentMetric.addValue(Number(value));
Copy link

@giancarlokc giancarlokc Jan 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@bobtfish What do you think of throwing an exception if we find that value is not a number?
Javascript has weird/grey areas when it comes to parsing. It might make more sense to delegate the parsing to whoever is calling putMetric.

Copy link
Author

Choose a reason for hiding this comment

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

Anything that isn’t just passing the value, quoting it and having cloudwatch silently drop it sounds fine to be honest.

I just didn’t want to make an API that currently fails silently start throwing exceptions, as that’s not a backwards compatible change (and so implies a new major version if semver is being used).

But if throwing an exception is the way that the AWS team wants to go then that’s fine with me!

} else {
this.metrics.set(key, new MetricValues(value, unit));
this.metrics.set(key, new MetricValues(Number(value), unit));
}
}

Expand Down
21 changes: 20 additions & 1 deletion src/logger/__tests__/MetricsLogger.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,25 @@ describe('successful', () => {
expect(actualMetric!.unit).toBe('None');
});

test('put metric with string corerces to Numeric', async () => {
// arrange
const expectedKey = faker.random.word();
const expectedValues = [faker.random.number(), faker.random.number()];

// act
logger.putMetric(expectedKey, `${expectedValues[0]}` as any);
logger.putMetric(expectedKey, `${expectedValues[1]}` as any);
await logger.flush();

// assert
expect(sink.events).toHaveLength(1);
const actualMetric = sink.events[0].metrics.get(expectedKey);
expect(actualMetric).toBeTruthy();
expect(actualMetric!.values).toStrictEqual(expectedValues);
expect(actualMetric!.unit).toBe('None');
});


test('can put metric with enum unit', async () => {
// arrange
const expectedKey = faker.random.word();
Expand Down Expand Up @@ -398,4 +417,4 @@ describe('failure', () => {
// assert
await expect(logger.flush()).rejects.toBeUndefined();
});
});
});