chore: refactor metrics util #136
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
- Coverage 99.35% 99.34% -0.02%
==========================================
Files 32 32
Lines 774 760 -14
Branches 206 206
==========================================
- Hits 769 755 -14
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| test('throws an error when detail is too long', () => { | ||
| const invalidDetail = 'a'.repeat(4001); | ||
| metrics.sendMetric('metricName', 0, invalidDetail); | ||
| expect(console.error).toHaveBeenCalledWith(`Detail for metric metricName is too long: ${invalidDetail}`); | ||
| jest.mocked(console.error).mockReset(); | ||
| expect(window.panorama).not.toHaveBeenCalled(); | ||
| expect(window.AWSC.Clog.log).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Duplicate test. Removed this copy, the other one stays
component-toolkit/src/internal/base-component/__tests__/metrics.test.ts
Lines 478 to 489 in e41ff41
| checkMetric(`awsui_DummyComponentName_d10`, { | ||
| o: 'main', | ||
| s: 'DummyComponentName', | ||
| t: 'default', | ||
| a: 'used', | ||
| f: 'react', | ||
| v: '1.0', | ||
| a: 'used', | ||
| s: 'DummyComponentName', | ||
| c: { props: {} }, | ||
| }); |
There was a problem hiding this comment.
The order of keys matters, because there is JSON.stringify down the line and we compare the serialized strings
| } | ||
|
|
||
| export function jsonStringify(detailObject: any) { | ||
| export function buildComponentMetricDetail( |
There was a problem hiding this comment.
Separated component metric details from generic base metric details (such as origin, theme, framework, version
| constructor(packageSource: PackageSettings); | ||
| constructor(packageSource: string, packageVersion: string); | ||
| constructor(...args: [PackageSettings] | [string, string]) { |
There was a problem hiding this comment.
Keeping dual signature until this one usage is migrated: https://github.com/cloudscape-design/components/blob/3bf581977a630226c8f574d5fa83a8e9c52b2a1c/src/internal/metrics.ts#L7
The rest of the code uses useComponentMetrics hook
just-boris
force-pushed
the
metrics-refactor
branch
from
6f44a0e to
96daed3
Compare
| if (!theme) { | ||
| // Metrics need to be initialized first (initMetrics) | ||
| console.error('Metrics need to be initialized first.'); | ||
| return; |
There was a problem hiding this comment.
Not needed anymore, because theme is always provided
just-boris
force-pushed
the
metrics-refactor
branch
from
96daed3 to
2e0dffb
Compare
just-boris
force-pushed
the
metrics-refactor
branch
from
2e0dffb to
904abf5
Compare
just-boris
requested review from
a team and
Al-Dani
and removed request for
a team and
avinashbot
| metrics.sendMetricOnce('my-event', 1); | ||
| metrics.sendMetricOnce('My-Event', 2); | ||
| metrics.sendOpsMetricValue('my-event', 1); | ||
| metrics.sendOpsMetricValue('My-Event', 2); |
There was a problem hiding this comment.
if it's just about casing, the value should be the same, otherwise it collides with the test above
just-boris
force-pushed
the
metrics-refactor
branch
from
f81862c to
d3bfb98
Compare
2 participants
Issue #, if available:
Description of changes:
Refactor this code
sendOpsMetricObject(name, detail)for objectssendOpsMetricValue(name, value)for number valuesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.