-
Notifications
You must be signed in to change notification settings - Fork 168
Cache metric ID and forward the metric info to series writer #1062
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.
There's no CHANGELOG entry, this change might be worth mentioning if it has an impact.
ce55435
to
ef8d4e3
Compare
39fca2c
to
197d681
Compare
@JamesGuthrie I've added an entry to CHANGELOG and update the benchmarks. I've left the old function in and added a new benchmark so we can compare the improvement. This is what I get on my machine:
I can always remove the old if we want to do it, I'll let Mat take a look and advise there. |
197d681
to
f3dffd7
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.
This is a very nice improvement. I think it's good to run the benchmark before and after the change and record diff in PR (as you have above). But I don't think we should keep the old functions around just for that purpose. Also I think that in a bunch of places (maybe all places) where you now pass around the metricID and tableName we should pass around a model.MetricInfo instead. That will make the code more future proof.
6827b90
to
3c47756
Compare
a392766
to
7d57fef
Compare
This is an optimization to remove the need to fetch metric ID and table name when storing new series data since we already fetch this info in a prior step of the process.
7d57fef
to
d9427d0
Compare
Description
This is an optimization to remove the need to fetch metric ID and
table name when storing new series data since we already fetch
this info in a prior step of the process.
Merge requirements
Please take into account the following non-code changes that you may need to make with your PR: