NormalizeMetricFamilies: Optimize memory usage

[ethanvc]

my first contribution to prometheus and this mr is going to make code more efficient.

[ArthurSens]

Hey, thanks for the PR! Could you sign the commits to make CI happy?

I'm assuming the efficiency improvement here is that we don't need to allocate memory for metric families with empty metric arrays, but could you elaborate if that's really what you're improving here?

[ethanvc]

Hey, thanks for the PR! Could you sign the commits to make CI happy?

I'm assuming the efficiency improvement here is that we don't need to allocate memory for metric families with empty metric arrays, but could you elaborate if that's really what you're improving here?

Sorry, already signed off.
There are serval minus impovements here:

1. do not need to allocate memory for names, which is a little large when we have lots of metrics.
2. do not need to allocate memory for empty metrics, but i think we may have very small amount of them.
3. do not need to searh the map for every mertrics, which save cpu times.

But i am not too sure if the key for metricFamiliesByName is always equal to MetricFamily.Name, which may crash if Name is nil after this change.

[ArthurSens]

By creating result before pruning metricFamilies with empty metrics, we might be over allocating memory 🤔

Not allocating space for names looks more relevant though, so that's good :)

But i am not too sure if the key for metricFamiliesByName is always equal to MetricFamily.Name, which may crash if Name is nil after this change.

Reading the code now, I can see that we get the metricFamily name from desc.fqName, which can't be nil or empty(See validation[1][2])

[bwplotka]

Thanks @ethanvc ! BTW what triggered this change? Some production performance issue or just looking at the code?

Generally, it's a premature optimization if we don't have a memory profile suggesting this code path inefficiency matters. It's also might be very ineffective change if we don't do any (at least) microbenchmarks to verify if we worsened or improved the efficiency (learning from my "Efficient Go" book btw) (:

With that preambule, let's check if, at least in theory, this makes sense:

1. do not need to allocate memory for names, which is a little large when we have lots of metrics.

Makes sense.

2. do not need to allocate memory for empty metrics, but i think we may have very small amount of them.

Your code still allocates memory for empty metrics, but indeed that should be fine. I don't know of any case where metric family might have empty metric (there might be, just I might not educated here) 🤔

3. do not need to searh the map for every mertrics, which save cpu times.

Not sure what do you mean by search, maybe you meant sort? I assume mf.Metric == 0 case is near 0% often, so on scale I don't think it matters. Generally you reduced from having 3 for loops to 1 for loop, but those are so small I doubt we will see considerable or any CPU improvement in practice. This is where only good benchmark would tell us.

But i am not too sure if the key for metricFamiliesByName is always equal to MetricFamily.Name, which may crash if Name is nil after this change.
Reading the code now, I can see that we get the metricFamily name from desc.fqName, which can't be nil or empty(See validation[1][2])

Nice, I am not worried about empty or nil case (see my suggestion). I more worried about map key is different than mf.Name. As @ArthurSens verified, this should be the case (for now 🙈 ).

Anyway, I think it's little harm to merge this (with my suggestion) WITHOUT micro-benchmarks, but I wouldn't be too excited here. I would even be not surprised if this function is now slower due to things we might miss (cacheability of the code, branching etc). Making any assumptions on such low level is guessing (:

It should allocate less (without benchmark that's again, just guess), but is it a lot? Even with 1000 metric families (extreme already) for []string that's only 24 + 16*1k bytes, so ~16KB of memory on every scrape (10-60s), easily garbage collected in between (I don' think this function is used more often)

LGTM, with below suggestion to be fixed first. Thanks @ArthurSens for detailed review 💪🏽