Skip to content
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

[pdata] Change outcome of Value collections accessors misuse #5034

Merged

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Mar 17, 2022

Change outcome of pdata.Value.MapVal() and pdata.Value.SliceVal() functions misuse. In case of type mismatch, they now return an invalid zero-initialized instance instead of a valid detached collection.

This PR implements the approach that was agreed upon in #4775 as a replacement for #4909

@dmitryax dmitryax requested review from a team and codeboten March 17, 2022 23:35
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #5034 (504fd0d) into main (d6b6251) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5034      +/-   ##
==========================================
+ Coverage   91.03%   91.07%   +0.03%     
==========================================
  Files         180      180              
  Lines       10808    10808              
==========================================
+ Hits         9839     9843       +4     
+ Misses        750      747       -3     
+ Partials      219      218       -1     
Impacted Files Coverage Δ
model/internal/pdata/common.go 94.26% <100.00%> (+0.71%) ⬆️

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 d6b6251...504fd0d. Read the comment docs.

@dmitryax dmitryax force-pushed the change-value-oneof-accessors branch 2 times, most recently from 2c9d1af to da0f165 Compare March 17, 2022 23:54
@dmitryax dmitryax changed the title [pdata] Change outcome of misuse of oneof collection accessors. [pdata] Change outcome of oneof collection accessors misuse Mar 17, 2022
@dmitryax dmitryax changed the title [pdata] Change outcome of oneof collection accessors misuse [pdata] Change outcome of Value collections accessors misuse Mar 18, 2022
@dmitryax dmitryax force-pushed the change-value-oneof-accessors branch from da0f165 to b324b96 Compare March 20, 2022 17:34
model/internal/pdata/common.go Outdated Show resolved Hide resolved
model/internal/pdata/common.go Outdated Show resolved Hide resolved
@dmitryax dmitryax force-pushed the change-value-oneof-accessors branch from b324b96 to 1ab0579 Compare March 21, 2022 16:06
@bogdandrutu
Copy link
Member

Failing tests :)

@dmitryax
Copy link
Member Author

dmitryax commented Mar 21, 2022

@bogdandrutu the failing test is checking the following input which unlikely to be caught in reality but I think we still need to handle it:

	orig := &otlpcommon.AnyValue{Value: &otlpcommon.AnyValue_KvlistValue{KvlistValue: nil}}
	m1 = Value{orig: orig}
	assert.EqualValues(t, AttributeMap{}, m1.MapVal())

To handle it, we would have to add another if check to the construction that you suggested, it'll look like this (Updated):

func (v Value) MapVal() AttributeMap {
	kvlist, ok := v.orig.GetValue().(*otlpcommon.AnyValue_KvlistValue)
	if !ok || kvlist.KvlistValue == nil {
		return AttributeMap{}
	}
	return newAttributeMap(&kvlist.KvlistValue.Values)
}

I run the benchmarks showing that original code of this PR appeared to be more performant:

Original:

BenchmarkAttributeValueMapAccessor-16    	1000000000	         0.7589 ns/op
BenchmarkAttributeValueArrayAccessor-16    	1000000000	         1.124 ns/op

Updated:

BenchmarkAttributeValueMapAccessor-16    	1000000000	         1.102 ns/op
BenchmarkAttributeValueArrayAccessor-16    	838671705	         1.350 ns/op

Change misuse outcome of `pdata.AttributeMap.MapVal()` and `pdata.AttributeValueSlice.SliceVal()`
  function calls. In case of type mismatch, they now return an invalid zero-initialized instance
  instead of returning a valid detached collection.
@dmitryax dmitryax force-pushed the change-value-oneof-accessors branch from 1ab0579 to 504fd0d Compare March 21, 2022 17:28
@dmitryax
Copy link
Member Author

I've brought back the original code for map and slice value getters for now

@bogdandrutu bogdandrutu merged commit 68374a6 into open-telemetry:main Mar 22, 2022
@dmitryax dmitryax deleted the change-value-oneof-accessors branch March 23, 2022 06:31
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
…-telemetry#5034)

Change misuse outcome of `pdata.AttributeMap.MapVal()` and `pdata.AttributeValueSlice.SliceVal()`
  function calls. In case of type mismatch, they now return an invalid zero-initialized instance
  instead of returning a valid detached collection.
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