Skip to content

Conversation

@bogdandrutu
Copy link
Member

This is important since we changed to use slice of values instead of slice of pointers.
After that change we returned a pointer to the memory block to allow users to change the value
of the returned Get or when ForEach, this has a problem if the user keeps a reference to
the returned value by the Get call, then applies any mutation that may move the memory around
Delete, Insert/Upsert the returned pointer may point to other block of memory (different
value in the map) or to a block that is no longer part of the slice, if the slice has to be moved.

@bogdandrutu bogdandrutu requested a review from a team October 28, 2020 07:42
This is important since we changed to use slice of values instead of slice of pointers.
After that change we returned a pointer to the memory block to allow users to change the value
of the returned Get or when ForEach, this has a problem if the user keeps a reference to
the returned value by the Get call, then applies any mutation that may move the memory around
Delete, Insert/Upsert the returned pointer may point to other block of memory (different
value in the map) or to a block that is no longer part of the slice, if the slice has to be moved.

Signed-off-by: Bogdan Drutu <[email protected]>
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #2021 into master will increase coverage by 0.01%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2021      +/-   ##
==========================================
+ Coverage   91.87%   91.88%   +0.01%     
==========================================
  Files         276      276              
  Lines       16495    16495              
==========================================
+ Hits        15155    15157       +2     
+ Misses        923      922       -1     
+ Partials      417      416       -1     
Impacted Files Coverage Δ
receiver/hostmetricsreceiver/internal/testutils.go 91.89% <66.66%> (ø)
consumer/pdata/common.go 96.68% <100.00%> (ø)
exporter/loggingexporter/logging_exporter.go 89.96% <100.00%> (ø)
translator/internaldata/metrics_to_oc.go 91.17% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 91.78% <0.00%> (+2.73%) ⬆️

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 36163b3...b2fda98. Read the comment docs.

@tigrannajaryan tigrannajaryan self-assigned this Oct 29, 2020
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do we have a similar problem with AttributeValue?

@bogdandrutu
Copy link
Member Author

@tigrannajaryan yes, and you saw that in the contrib when I moved the delete, will try to find a solution for the AttributeValue

@bogdandrutu bogdandrutu merged commit 62ef585 into open-telemetry:master Oct 29, 2020
@bogdandrutu bogdandrutu deleted the rmstringvalue branch October 29, 2020 19:21
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants