Skip to content

[pdata] Introduce runtime safeguards to catch incorrect pdata mutations #8494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Sep 20, 2023

This change introduces an option to enable runtime assertions to catch unintentional pdata mutations in components that are claimed as non-mutating pdata. Without these assertions, runtime errors may still occur, but thrown by unrelated components, making it very difficult to troubleshoot.

For now, this doesn't change the default behavior. It just introduces a new method on [Metrics/Traces|Logs].Shared() that returns pdata marked as shared. The method will be applied to fan-out consumers in the next PR.

Later, if we want to remove the need of MutatesData capability, we can introduce another method [Metrics/Traces|Logs].Exclusive() which returns a copy of the pdata if it's shared.

This change unblocks the 1.0 release by implementing the original solution proposed by @bogdandrutu in #6794. Going forward, we may introduce a copy-on-write solution that doesn't require the runtime assertions. That will likely be part of the 2.0 release.

@dmitryax dmitryax requested review from a team and mx-psi September 20, 2023 06:10
@dmitryax dmitryax force-pushed the pdata-panic-mutation branch 5 times, most recently from df2fb46 to 14d4455 Compare September 20, 2023 06:42
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
pdata/internal/generated_wrapper_byteslice.go 100.00% <100.00%> (ø)
pdata/internal/generated_wrapper_float64slice.go 100.00% <100.00%> (ø)
...internal/generated_wrapper_instrumentationscope.go 100.00% <100.00%> (ø)
pdata/internal/generated_wrapper_resource.go 100.00% <100.00%> (ø)
pdata/internal/generated_wrapper_uint64slice.go 100.00% <100.00%> (ø)
pdata/internal/state.go 100.00% <100.00%> (ø)
pdata/internal/wrapper_logs.go 100.00% <100.00%> (ø)
pdata/internal/wrapper_map.go 100.00% <100.00%> (ø)
pdata/internal/wrapper_metrics.go 100.00% <100.00%> (ø)
pdata/internal/wrapper_slice.go 100.00% <100.00%> (ø)
... and 68 more

📢 Thoughts on this report? Let us know!.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @dmitryax, have you run benchmarks to compare the results of this change?

@dmitryax dmitryax force-pushed the pdata-panic-mutation branch from 14d4455 to 6a06d44 Compare September 26, 2023 18:43
@dmitryax
Copy link
Member Author

dmitryax commented Sep 26, 2023

@codeboten the performance should not be affected because the additional field is a part of a struct that stays on the memory stack. I added some benchmarks from my previous work to confirm it:

Before:

BenchmarkTracesUsage-10    	    1832	    613221 ns/op	   50631 B/op	    3409 allocs/op
BenchmarkMetricsUsage-10    	     506	   2247812 ns/op	  115778 B/op	    8946 allocs/op
BenchmarkLogsUsage-10    	    5146	    224939 ns/op	   26323 B/op	    1645 allocs/op

After:

BenchmarkTracesUsage-10    	    1795	    615730 ns/op	   50629 B/op	    3409 allocs/op
BenchmarkMetricsUsage-10    	     512	   2288719 ns/op	  115777 B/op	    8946 allocs/op
BenchmarkLogsUsage-10    	    5469	    217728 ns/op	   26321 B/op	    1645 allocs/op

@dmitryax dmitryax force-pushed the pdata-panic-mutation branch 2 times, most recently from 930fc3d to 5738a36 Compare September 26, 2023 23:52
@dmitryax
Copy link
Member Author

dmitryax commented Sep 26, 2023

Updated PR to add coverage for panics. Had to make some related chores in pdatagen that I moved to another PR #8537

@dmitryax dmitryax force-pushed the pdata-panic-mutation branch 5 times, most recently from ad0779d to 2cd811d Compare September 27, 2023 18:47
@dmitryax dmitryax force-pushed the pdata-panic-mutation branch 4 times, most recently from da33394 to a1692b7 Compare October 4, 2023 23:55
@dmitryax dmitryax force-pushed the pdata-panic-mutation branch 2 times, most recently from 3621036 to ad98f6d Compare October 5, 2023 21:55
This change introduces an option to enable runtime assertions to catch unintentional pdata mutations in components that are claimed as non-mutating pdata. Without these assertions, runtime errors may still occur, but thrown by unrelated components, making it difficult to troubleshoot the problem.

For now, this doesn't change the default behavior. It just introduces a new method on [Metrics/Traces|Logs].Shared. The method will be applied to fan out consumers in the next PR.

This change unblocks the 1.0 release of pdata. Going-forward we may introduce copy-on-write solution which won't require the runtime assertions. That will be likely part of 2.0 release.
@dmitryax dmitryax force-pushed the pdata-panic-mutation branch from ad98f6d to 9f7ce90 Compare October 5, 2023 23:15
@bogdandrutu bogdandrutu merged commit 7ee0b28 into open-telemetry:main Oct 6, 2023
@github-actions github-actions bot added this to the next release milestone Oct 6, 2023
@dmitryax dmitryax deleted the pdata-panic-mutation branch October 6, 2023 16:15
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.

3 participants