-
Notifications
You must be signed in to change notification settings - Fork 8
FFM-11022 Applies standard Metrics Enhancements #112
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
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.
Just some comments/suggestions to help with maintainability. No blockers
private MAX_EVALUATION_ANALYTICS_SIZE = 10000; | ||
private MAX_TARGET_ANALYTICS_SIZE = 100000; |
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 THINK you can set these as static
. Will make zero difference, but ¯_(ツ)_/¯
|
||
clonedEvaluationAnalytics.forEach((event) => { |
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.
TOTALLY not an issue, but instead of creating a variable and then using a forEach
to push to that variable. You can just swap to map
and assign in one.
clonedEvaluationAnalytics.forEach((event) => { | |
const metricsData: MetricsData[] = clonedEvaluationAnalytics.map((event) => { |
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 like 🚀 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.
Dang - the map
function isn't available on Map
types. I'd need to convert clonedEvaluationAnalytics
to an array. I'll opt to leave it as is. But in principle I agree with the functional style here.
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.
Ah man... annoying. You could spread into an array or use Array.from
. Don't worry about it, it's fine as-is.
clonedEvaluationAnalytics.forEach((event) => { | |
const metricsData: MetricsData[] = [...clonedEvaluationAnalytics.values()].map((event) => { |
} | ||
}); | ||
|
||
clonedTargetAnalytics.forEach((target) => { |
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.
Same here. Also totally a non-issue.
clonedTargetAnalytics.forEach((target) => { | |
const targetData: TargetData[] = clonedTargetAnalytics.map((target) => { |
src/metrics.ts
Outdated
const td: TargetData = { | ||
identifier: target.identifier, | ||
name: targetName, | ||
attributes: targetAttributes, | ||
}; | ||
targetData.push(td); |
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 you do want to keep as a forEach
/push
, then you might as well just inline this.
const td: TargetData = { | |
identifier: target.identifier, | |
name: targetName, | |
attributes: targetAttributes, | |
}; | |
targetData.push(td); | |
targetData.push({ | |
identifier: target.identifier, | |
name: targetName, | |
attributes: targetAttributes, | |
}); |
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.
Done
Co-authored-by: Kevin Nagurski <[email protected]>
Co-authored-by: Kevin Nagurski <[email protected]>
Co-authored-by: Kevin Nagurski <[email protected]>
What
Splits evaluation and target metrics into their own caches:
globalTarget
, this meant that we would miss targets if the evaluation didn't change.Implement seen targets:
Testing