Replace InitEmptyWithCapacity with EnsureCapacity and Clear

[bogdandrutu]

Updates #2488

Signed-off-by: Bogdan Drutu [email protected]

[bogdandrutu]

/cc @Aneurysm9

[codecov]

Codecov Report

Merging #2845 (2bfee7e) into main (03c7bf9) will decrease coverage by 0.00%.
The diff coverage is 94.28%.

❗ Current head 2bfee7e differs from pull request most recent head 13f453f. Consider uploading reports for the commit 13f453f to get more accurate results

@@            Coverage Diff             @@
##             main    #2845      +/-   ##
==========================================
- Coverage   91.66%   91.66%   -0.01%     
==========================================
  Files         312      312              
  Lines       15317    15308       -9     
==========================================
- Hits        14041    14032       -9     
  Misses        870      870              
  Partials      406      406              

Impacted Files	Coverage Δ
...eceiver/internal/scraper/processscraper/process.go	80.00% <77.77%> (-4.62%)	⬇️
consumer/pdata/common.go	98.93% <100.00%> (+0.01%)	⬆️
processor/resourceprocessor/resource_processor.go	100.00% <100.00%> (ø)
translator/internaldata/oc_to_metrics.go	91.24% <100.00%> (ø)
translator/internaldata/oc_to_resource.go	100.00% <100.00%> (ø)
translator/internaldata/oc_to_traces.go	90.21% <100.00%> (ø)
translator/trace/jaeger/jaegerproto_to_traces.go	90.67% <100.00%> (ø)
translator/trace/jaeger/jaegerthrift_to_traces.go	89.79% <100.00%> (ø)
translator/trace/zipkin/zipkinv2_to_traces.go	89.91% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03c7bf9...13f453f. Read the comment docs.

[tigrannajaryan]

Please check carefully if change of semantic affects the code. If it does not affect it change the function signatures to make it clear that we are dealing with new slices and they are already empty. Otherwise the code looks wrong.

[tigrannajaryan]

Append may be faster to increase the capacity, assuming memory manager has a way to extend the allocated block without reallocation (but I did not verify): https://github.com/golang/go/wiki/SliceTricks#extend
It may be worth benchmarking.

[bogdandrutu]

We can play with the internal implementation.

[tigrannajaryan]

Change in behvaior if labelsMap was not empty before. Is this guaranteed? If this is the case I suggest changing setLabelsMap to return labelsMap pdata.StringMap instead of accepting it as a parameter.

[bogdandrutu]

Returning is not a good pattern for the way we want users to use our pdata. Returning means I need to have a SetLablesMap which we don't have because we want users to construct things top-down to avoid extra memory allocation when copying to the parent struct.

In this case, as in all the other cases we start with an empty object during conversion, so there is no problem with having previous values.

[bogdandrutu]

In contrib I plan to replace with Clear + EnsureCapacity so I don't have to manually check all usages.

[tigrannajaryan]

It is not apparent from setLabelsMap signature that labelsMap is empty. By looking at setLabelsMap code only it is not possible to tell if the code is correct. To know that the code is correct you have to analyse the call site where setLabelsMap.

What do you think about calling Clear() here too (like you want to do in contrib), or declare the requirement in setLabelsMap doccomment that labelsMap must be empty? I think calling Clear() is fine, it should have very little cost.

[bogdandrutu]

Ok, just to make the reviewer happy :)

[tigrannajaryan]

:-)

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.