Generic code template for consolidated element logic #95
Conversation
| require.Equal(t, testCounter.CounterVal, e.values[0].counter.Sum()) | ||
| require.Equal(t, int64(1), e.values[0].counter.Count()) | ||
| require.Equal(t, int64(0), e.values[0].counter.SumSq()) | ||
| require.Equal(t, testCounter.CounterVal, e.values[0].aggregation.Sum()) |
There was a problem hiding this comment.
Should make all the Sum() Count() ... functions private in those aggregation types since only ValueOf() is being used by element now? And these test here should not verify the results of functions like Sum() or Count() but should verify the result of ValueOf()?
There was a problem hiding this comment.
Sure can do in the next PR, would like to only include the code-gen related changes in this PR
aggregator/elem_base.go
Outdated
| ) { | ||
| if useDefaultAggregation { | ||
| e.quantiles = aggTypesOpts.TimerQuantiles() | ||
| e.isQuantilesPooled = false |
There was a problem hiding this comment.
can this check be replaced with just e.quantilesPool = nil?
There was a problem hiding this comment.
Hm not sure I understand, you need to check isUseDefaultAggregation to determine how to set e.quantilesPool
There was a problem hiding this comment.
think you can just do e.quantilesPool = nil here since you are resetting with the aggTypesOpts
There was a problem hiding this comment.
Are you suggesting removing e.isQuantilesPooled?
There was a problem hiding this comment.
Hm I think it's less readable that way to save one boolean var.
if useDefaultAggregation {
e.quantiles = aggTypesOpts.TimerQuantiles()
e.quantilesPool = nil
return
}
var (
quantilesPool = aggTypesOpts.QuantilesPool()
isQuantilesPooled bool
)
e.quantiles, isQuantilesPooled = aggTypes.PooledQuantiles(quantilesPool)
if isQuantilesPooled {
e.quantilesPool = quantilesPool
} else {
e.quantilesPool = nil
}
There was a problem hiding this comment.
sorry, did not realize that you need the extra logic to check on the isQuantilesPooled, I'm ok either way in that case
aggregator/generic_elem.go
Outdated
| // GenericElem is an element storing time-bucketed aggregations. | ||
| type GenericElem struct { | ||
| elemBase | ||
| genericElemBase |
There was a problem hiding this comment.
both elemBase and genericElemBase sound very generic and they sounded similar, maybe rename genericElemBase to genericTypedElemBase?
There was a problem hiding this comment.
Hm I initially called it metricTypeElemBase but landed on just genericElemBase. I can rename to typeSpecificElemBase sure.
| // NB(cw) Use default suffix slice for faster look up. | ||
| suffixes := e.DefaultAggregationTypeStrings(e.aggTypesOpts) | ||
| aggTypes := e.DefaultAggregationTypes(e.aggTypesOpts) | ||
| agg.Lock() |
There was a problem hiding this comment.
did we have this lock before? why do we need it now?
There was a problem hiding this comment.
Yeah it was there before (in Consume in elem.go). This is simply moving it into the processValues method.
| return | ||
| } | ||
|
|
||
| agg.Lock() |
There was a problem hiding this comment.
what's the thinking about merging this with the default case?
There was a problem hiding this comment.
Sure can do in the next PR, would like to only include the code-gen related changes in this PR
cc @cw9 @jeromefroe
This PR factors out common code for aggregation elems for different aggregation elements and uses genny to generate type-specific implementations for reduced code duplication and maintenance overhead.
This PR is against dev branch.