-
Notifications
You must be signed in to change notification settings - Fork 135
feat(worker): Custom Metrics #1705
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
Conversation
33af2ea
to
4eba51d
Compare
4eba51d
to
8271248
Compare
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.
Looks really clean & thorough. I didn't do an incredibly close analysis, but overall it looks great and the tests have excellent coverage.
* | ||
* @param tags Tags to append to existing tags. | ||
*/ | ||
withTags(tags: MetricTags): MetricCounter; |
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.
Why not put withTags
on Metric
?
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 would be the return type of that function if defined on the parent class? Unfortunately, TS doesn't have anything like the Self
type.
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.
@mjameswh I noticed this while browsing through previous PRs to understand what work has been done / yet to be completed. (I'm trying to understand if we can update the SDK metrics to emit native histograms instead of classic ones with explicit buckets).
Couldn't help suggest though, if you want to add withTags
to the base interface but ensure it returns the correct subtype, you could possibly use the "curiously recursive template pattern" (CRTP) pattern:
interface Metric<T extends Metric<T>> {
// omitted
withTags(tags: MetricTags): T;
}
interface MetricCounter extends Metric<MetricCounter> {
// omitted
add(value: number, extraTags?: MetricTags): void;
}
const counter = {} as MetricCounter;
const counterWithTags = counter.withTags({"custom.label": "xyz"});
counterWithTags.add(5) // type: MetricCounter
I was always quite fond of that in C++. Not sure if this is recommended in TS, or how generally extensible it is 😅
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.
Curiously Recursive Template Pattern… That's a funny name 😄. I remember using that concept in several designs I did years ago, mostly in Java, but never heard the name of that pattern. Thanks for the reference!
Now, about this specific case… Yes, that would certainly work here. In fact, I had briefly consider that option following Sushisource's comment. But I had dismissed it because I don't think there sufficient win in this case to justify introducing a new idiom to the code base. I mean, that would simply save redefinition of the withTags
methods exactly three times, on MetricCounter
, MetricHistogram
and MetricGauge
, which isn't much, and I really can't think of any case where one would be likely to call withTags
on a non-identified subtype of Metric
.
But happy to revisit, now or at a later time, if anyone can think of an argument in favor of stronger generalization of these types. There should be nothing preventing retrofitting the existing types to that pattern, if ever justified.
What was changed
Example usage:
From a workflow
From an activity