[WIP] Propose new API for aggregating metrics #54
Conversation
| expect(evt.getDimensions()[0]).toBe([ | ||
| { ...DEFAULT_DIMENSIONS, client }, | ||
| { ...DEFAULT_DIMENSIONS, pageType }, | ||
| { ...DEFAULT_DIMENSIONS, client, pageType }, |
There was a problem hiding this comment.
This is the part I'm not certain of. One question I have is what are users most likely to do? Do users prefer the default dimensions are included most of the time or are users overwriting them more often than not.
There was a problem hiding this comment.
This is a tough one 😄
I don't use the default dimensions and having true by default will make me very happy. Nonetheless, this is a personal preference and I guess we should reason in terms of user expectations.
One way to see it could be to keep the same behaviour that putMetric and putDimensions have perhaps? Which means, the default should be false. Also, given the wording of the property is stripDefaultDimensions it entails that by default is disabled and if you want to activate it you must specify true. The opposite would have been keepDefaultDimensions with default set to false, and activation to true.
For people that don't use the default dimensions, I would expect to either set stripDefaultDimensions explicitly to true every time I call putMetricWithDimensions, or call setDimensions() with empty parameter once at the beginning and then regardless of what the value of stripDefaultDimensions is, the default shouldn't be set because they have been removed for the whole session.
There was a problem hiding this comment.
Right now I’m leaning towards the proposal as is. I think it may make sense to offer a global configuration to disable default dimensions entirely. I suspect you either want them or you don’t and you may want that applied uniformly.
src/logger/MetricsLogger.ts
Outdated
| @@ -19,6 +19,14 @@ import { IEnvironment } from '../environment/IEnvironment'; | |||
| import { MetricsContext } from './MetricsContext'; | |||
| import { Unit } from './Unit'; | |||
|
|
|||
| type Metrics = { [key: string]: number | number[]; }; | |||
There was a problem hiding this comment.
This type declaration doesn't allow the definition of the "unit" (e.g. count, milliseconds, etc.).
I was hoping to set something like:
{
Name: 'ResponseTime',
Value: 59,
Unit: Unit.Milliseconds
}Also, multiple metrics may apply to the same dimensions, shouldn't we define this as an array of metric items as discussed in #46?
Example:
m.putMetricWithDimensions({
metrics: [
{
Name: 'ResponseTime',
Value: 322,
Unit: Unit.Milliseconds
},
{
Name: 'RequestCount',
Value: 1,
Unit: Unit.Count
}
],
namespace: '/example/namespace',
dimensions: [{ PageType: pageType }],
stripDefaultDimensions: true
});There was a problem hiding this comment.
Yeah agree on unit. The idea here was that the key would be used as the metric name which naturally supports multiple distinct metrics. I don’t recall exactly why I proposed it this way, probably because it was less verbose and easier to read but once you add units to the mix it breaks down. Happy to go with this.
|
@jaredcnance any thoughts on the feedbacks? |
For full transparency, I’m targeting catching up on this PR in 2-3 weeks. |
Fantastic, thanks very much @jaredcnance |
|
Hi @jaredcnance any update on this PR? |
|
Any likelihood for this to proceed? |
|
Hi @markkuhn and @Himtanaya just wanted to bring to your attention this old PR to hear your opinion about it and understand a way forward. For full context, this #46 is the issue that raised this PR. Thanks for your time. |
Issue: #46
This PR serves to facilitate further discussion about the shape that the new API discussed in #46 should take.