feat: add stats view implementation #89
Conversation
eduardoemery
force-pushed
the
stats_view
branch
2 times, most recently
from
56dcb28
to
a443f62
Compare
eduardoemery
force-pushed
the
stats_view
branch
from
a443f62
to
b1ab2fe
Compare
| constructor( | ||
| name: string, measure: Measure, aggregation: AggregationType, | ||
| tagKeys: string[], description?: string) { | ||
| throw new Error('Not Implemented'); | ||
| tagsKeys: string[], description?: string, bucketBoudaries?: number[]) { |
There was a problem hiding this comment.
bucketBoundaries (you can do a global replace)
There was a problem hiding this comment.
Thanks! Updated here and all occurrences.
| recordMeasurement(measurement: Measurement) { | ||
| // Checks if measurements has all tags in views | ||
| for (const tagKey of this.columns) { | ||
| if (!Object.keys(measurement.tags).find((key) => key === tagKey)) { |
There was a problem hiding this comment.
Prefer some over find here, as the return result could be an empty string. Of course any empty string key is not a real tag, but this will at least "work" if some is used instead.
There was a problem hiding this comment.
Great suggestion. Updated.
| */ | ||
| private encodeTags(tags: Tags): string { | ||
| const encodedTags = Object.keys(tags).sort().reduce((strTags, tagKey) => { | ||
| return strTags + `${tagKey}:${tags[tagKey]},`; |
There was a problem hiding this comment.
Is : a reasonable delineator here?
There was a problem hiding this comment.
Hmm, I used : since that's what is used in a JSON. So I separated the key from value with a : and the key:value pairs with a ,. But I'm up for suggestions.
There was a problem hiding this comment.
I see. It would be safer to take advantage of the restrictions on tag keys/values. For example, we can use non-printable characters as delineators, since both keys and values must use printable characters. That way there is guaranteed to be no name collision in weird cases where the value contains : or ,.
Also, map + join might be clearer than reduce.
There was a problem hiding this comment.
Great point. I added a verification for the tags keys and values before recording. Also, changed to map + join, it's better 😄
eduardoemery
force-pushed
the
stats_view
branch
from
eca71e0
to
3354bff
Compare
|
Made two small changes as we discussed:
|
eduardoemery
force-pushed
the
stats_view
branch
2 times, most recently
from
58a4d71
to
923a9f2
Compare
| * @param str The string to be checked | ||
| */ | ||
| private invalidString(str: string): boolean { | ||
| return RegExp(`[${LOW_LIMIT_CHAR}-${HIGH_LIMIT_CHAR}]`).test(str) || |
There was a problem hiding this comment.
Nit: I would suggest creating this RegExp in the file scope or as a static field so it doesn't have to get constructed on every call to invalidString. Its value is static anyway. (Same for next line.)
Also, would /[^\u0020-\u007e]/ work as a regex to encompass both?
There was a problem hiding this comment.
I opted for RegExp() for readability when passing LOW_LIMIT_CHAR and HIGH_LIMIT_CHAR but hadn't considered the performance issue. I think it is way better with /[^\u0020-\u007e]/. Thanks again for the tips!
eduardoemery
force-pushed
the
stats_view
branch
from
923a9f2
to
148f53c
Compare
* feat: add stats view implementation * refactor(fix): changes to address review comments * feat: add method to access view's columns * feat: add tag format verification
* feat: add stats view implementation * refactor(fix): changes to address review comments * feat: add method to access view's columns * feat: add tag format verification
* feat: add stats view implementation * refactor(fix): changes to address review comments * feat: add method to access view's columns * feat: add tag format verification
Add stats view implementation and its tests.