-
Notifications
You must be signed in to change notification settings - Fork 821
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.
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: implement labelset #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM! Left a few questions/comments
Codecov Report
@@ Coverage Diff @@
## master #463 +/- ##
=========================================
+ Coverage 91.6% 94.81% +3.2%
=========================================
Files 124 132 +8
Lines 5864 6206 +342
Branches 551 558 +7
=========================================
+ Hits 5372 5884 +512
+ Misses 492 322 -170
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments.
@@ -23,5 +23,5 @@ const COMMA_SEPARATOR = ','; | |||
* @returns The hashed label values string. | |||
*/ | |||
export function hashLabelValues(labelValues: string[]): string { | |||
return labelValues.sort().join(COMMA_SEPARATOR); | |||
return labelValues.join(COMMA_SEPARATOR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed sort()
here because per the specs, meter.labels(...)
function is called before calling any instrument function, thus labels passing to any instrument should be already sorted based on keys. I think we should keep hashLabelValues
function to map values and the handle that created the certain values. @mayurkale22 WDYT? Also this may create mistake on user's end when user misuses instrument's method without calling meter.labels(...)
and pass an unsorted labels directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may create mistake on user's end when user misuses instrument's method without calling meter.labels(...) and pass an unsorted labels directly.
This is a valid point, I am gonna give another read on specs. Let us know if you have a better solution to handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments, overall looks like things are coming together.
@@ -23,5 +23,5 @@ const COMMA_SEPARATOR = ','; | |||
* @returns The hashed label values string. | |||
*/ | |||
export function hashLabelValues(labelValues: string[]): string { | |||
return labelValues.sort().join(COMMA_SEPARATOR); | |||
return labelValues.join(COMMA_SEPARATOR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may create mistake on user's end when user misuses instrument's method without calling meter.labels(...) and pass an unsorted labels directly.
This is a valid point, I am gonna give another read on specs. Let us know if you have a better solution to handle this.
let labelSetKey = keys.reduce((result, key) => { | ||
if (result.length > 2) { | ||
result += ','; | ||
} | ||
result = result + key + ':' + labels[key]; | ||
return result; | ||
}, '|#'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generate labelSetKey as "|#key1:val1,key2:val2", not sure if we need the prefix "|#", just be consistent with GO here.
* @param labelValues The list of the label values. | ||
* @returns The hashed label values string. | ||
*/ | ||
export function hashLabelValues(labelValues: string[]): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With labelSetKey, there's no need to have this function.
export type Labels = Record<string, string>; | ||
|
||
/** | ||
* Canonicalized labels with an unique string labelSetKey. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Canonicalized labels with an unique string labelSetKey. | |
* Canonicalized labels with a unique string labelSetKey. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If labelSetKey
is renamed, would need to adjust this based on that :)
* Canonicalized labels with an unique string labelSetKey. | ||
*/ | ||
export interface LabelSet { | ||
labelSetKey: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about calling this something like encoded
? I think we should avoid having the word "key" or "value" in this to prevent someone from thinking this is part of the actual dictionary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer something like identifier
that doesn't have as much implied meaning as encoded
. To me, encoded
implies that it is a decodable version of the LabelSet, where I don't think that is actually guaranteed by the spec. I think it's just meant to be a unique identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dyladan is correct in that it is meant to be a unique identifier. We are using encoded
in Python as well as Go implementation however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am okay with identifier
as well. It diverges from other SDKs, but I think it is more clear
result = result + key + ':' + labels[key]; | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result += key + ':' + labels[key];
return result;
or maybe even just
return result += key + ':' + labels[key];
@@ -13,15 +13,3 @@ | |||
* See the License for the specific language governing permissions and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe delete Utils.ts
?
@@ -69,3 +69,13 @@ export const DEFAULT_METRIC_OPTIONS = { | |||
labelKeys: [], | |||
valueType: ValueType.DOUBLE, | |||
}; | |||
|
|||
export class LabelSet implements LabelSet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few suggestions:
- Add JSDoc comment
- Move
LabelSet
to separate module (LabelSet.ts) (?) LabelSet implements LabelSet
looks typo or is this intentional stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
*/ | ||
export class CounterHandle extends BaseHandle implements types.CounterHandle { | ||
constructor( | ||
_labelSet: types.LabelSet, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this isn't private
should this just be named labelSet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@draffensperger Yeah that makes sense, then should the _data
above be named as data
? thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, no _data
is an actual member of the class. We underscore prefix those so that it's clear in the generated JS that they are private. But here labelSet
is only a parameter, not a property of the object.
} | ||
return (result += key + ':' + labels[key]); | ||
}, '|#'); | ||
let sortedLabels = {} as types.Labels; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this work as let sortedLabels: types.Labels = {}
? That's generally a better pattern because it makes sure that the object actually fits the type rather than doing a forceful cast to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@draffensperger Thanks, will fix it.
*/ | ||
labels(labels: types.Labels): types.LabelSet { | ||
let keys = Object.keys(labels).sort(); | ||
let identifier = keys.reduce((result, key) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this imply that we don't support ,
and :
in metric label values? Do we enforce that validation anywhere and document it?
Which problem is this PR solving?
Short description of the changes