[pdata] Add AsReadOnly function to pcommon.Map and pcommon.Slice

[edmocosta]

trafficstars

Description

This PR adds the function AsReadOnly to the pcommon.Map and pcommon.Slice, which goal is to allow API users to create read-only shallow copies of those objects.

This utility is specially useful when those types are passed around, and no modification is expected to be performed on them. For example, a few OTTL functions are currently leveraging those objects mutability to set their values (see), causing problems and unexpected behavior. Entirely copying them could be an alternative to avoid that, but it's too expensive.

Testing

Unit tests

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 91.48%. Comparing base (8568c97) to head (79d1eaf). :warning: Report is 185 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #12995   +/-   ##
=======================================
  Coverage   91.48%   91.48%           
=======================================
  Files         506      506           
  Lines       28557    28563    +6     
=======================================
+ Hits        26125    26131    +6     
  Misses       1917     1917           
  Partials      515      515           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[edmocosta]

it seems the contrib-tests (processor/deltatocumulativeprocessor) are hanging as it uses some equality check that does not handle infinite recursion (see here). It basically goes through all exported methods of the struct, finds the new AsReadOnly function, and starts an infinite looping trying to reach the end of the struct hierarchy, calling AsReadOnly over and over again.

[edmocosta]

Do you have any thoughts on this @open-telemetry/collector-approvers? If you agree with this feature, I can manage pinging processor/deltatocumulativeprocessor codeowners so we can fix that failing test I mentioned here.

[edmocosta]

I don't see a good enough reason here to open this functionality to any sub-type and to possibly limit our future development of this feature because of this.

I don't see how this functionality would limit future development, it's leveraring what's already implemented, and I think any behavior changes on that API would be considered a breaking change anyway.

I'm fine if you folks disagree on adding it, the goal was avoiding coping/wrapping maps & slices over and over again when read-only instances are desired (probably the same use-case that led implementing the existing logic). IMO, those data structures, especially maps, already have some overhead for accessing specific values, and adding another layer for copying it before accessing them would make it even slower. Anyway, that's an OTTL issue, and if this feature is not added, we will need to find another alternative for that @TylerHelmuth @evan-bradley.

Thanks.

[github-actions[bot]]

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

[github-actions[bot]]

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

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.