Added support for list values for a key in processor #1936

isgoutham · 2020-10-12T04:56:05Z

Updated/Added cases in opentelemetry-collector/internal/processor/filterhelper/filterhelper.go for handling list of interfaces and map

Updated Match func in opentelemetry-collector/internal/processor/filtermatcher/attributematcher.go for validating AttributeValueARRAY data

Description: Support for list of values for a key for attribute processor
Link to tracking Issue: #1935

Updated/Added cases in opentelemetry-collector/internal/processor/filterhelper/filterhelper.go for handling list of interfaces and map Updated Match fn in opentelemetry-collector/internal/processor/filtermatcher/attributematcher.go for validating AttributeValueARRAY data

linux-foundation-easycla · 2020-10-12T04:56:08Z

The committers are authorized under a signed CLA.

✅ Goutham Ichapuram (cc919b5)

isgoutham · 2020-10-12T04:59:33Z

internal/processor/filtermatcher/attributematcher.go

 				return false
+			default:
+				if !attr.Equal(*property.AttributeValue) {


handling at attr.Equal(*property.AttributeValue) would be ideal case. But attr, exist := attrs.Get(property.Key) returns String type for array values under property.Key. And further it always goes under string case execution

Can you point to the code where the return value is string?

Here Get func. Though the property.AttributeValue is of ARRAY type (since NewAttributeValueRaw() returns NewAttributeValueArray as per my logic), the return value of Get method is of string type.

Otherwise handling at Equal func would be ideal. In fact, there is a // TODO: handle MAP and ARRAY data types. I have tried adding these cases, but attr is of string type and Equal method is falling under string case.

I have tried using attr.SetArrayVal() but this is changing whole attributes as array.

Anything should i be considering for these scenarios?

codecov · 2020-10-12T05:02:30Z

Codecov Report

Merging #1936 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1936      +/-   ##
==========================================
+ Coverage   91.41%   91.42%   +0.01%     
==========================================
  Files         284      284              
  Lines       16788    16808      +20     
==========================================
+ Hits        15347    15367      +20     
  Misses       1009     1009              
  Partials      432      432

Impacted Files	Coverage Δ
internal/processor/filterhelper/filterhelper.go	`100.00% <100.00%> (ø)`
...ternal/processor/filtermatcher/attributematcher.go	`100.00% <100.00%> (ø)`
translator/internaldata/resource_to_oc.go	`91.78% <0.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 031521d...2e307bc. Read the comment docs.

bogdandrutu · 2020-10-13T01:14:53Z

internal/processor/filterhelper/filterhelper.go

+	return av
+}
+
+func fromVal(val interface{}) pdata.AttributeValue {


Can we avoid duplicate code between this func and NewAttributeValueRaw?

Noted and updated

bogdandrutu · 2020-10-13T01:15:04Z

internal/processor/filterhelper/filterhelper.go

+	case map[string]interface{}:
+		return fromMap(v)
+	}
+	panic("data type is not supported in fromVal()")


We should not panic

Noted and updated

james-bebbington · 2020-10-13T01:31:06Z

internal/processor/filterhelper/filterhelper.go

+		return pdata.NewAttributeValueInt(int64(v))
+	case float64:
+		return pdata.NewAttributeValueDouble(v)
+	case []interface{}:


I'm not sure how this code is intended to be used, but note data of type []string or []MyStruct won't meet this condition (same for the map case below)

https://play.golang.org/p/ZRosREV0Fa2

Its a slice case, tried here
https://play.golang.org/p/9Q4YHsuqVJ7

covered in test cases too

this is out of scope for this PR

isgoutham · 2020-10-20T10:01:13Z

Hi @bogdandrutu , could you please check this.

isgoutham · 2020-10-22T04:18:19Z

Hi @bogdandrutu could you please review

asldevi · 2020-10-23T05:51:26Z

@bogdandrutu is there any blocker for this?
we're trying to do 100% sampling for all errors and right now the only is to have a processor for each of the error code. Looks like this PR would be a perfect thing for us.

tigrannajaryan · 2020-10-26T16:01:11Z

internal/processor/filtermatcher/attributematcher.go

+			propertyAttrValue := property.AttributeValue
+			switch propertyAttrValue.Type() {
+			// Case when Attribute Values are of list values.
+			case pdata.AttributeValueARRAY:
+				for i := 0; i < propertyAttrValue.ArrayVal().Len(); i++ {
+					// Check attr value is exists in the AttributeValue array.
+					// Equal checks for any of int, string, bool and double.
+					if attr.Equal(propertyAttrValue.ArrayVal().At(i)) {
+						return true
+					}
+				}
 				return false
+			default:


Instead of doing this here I suggest that we move this code to attr.Equal. It already has a TODO comment that expects this code to be there.

Thats right. I tried doing that earlier.
#1936 (comment)

tigrannajaryan

Looks like I misunderstood your proposal #1935
I will comment there.

tigrannajaryan · 2020-10-28T13:45:53Z

Closing this PR as per discussion in #1935 since this can be done using existing functionality.

…pen-telemetry#1936)

isgoutham requested review from bogdandrutu, dmitryax, james-bebbington, nilebox, owais, pjanotti and tigrannajaryan as code owners October 12, 2020 04:56

isgoutham commented Oct 12, 2020

View reviewed changes

isgoutham mentioned this pull request Oct 12, 2020

Supporting Key and a List of Values in the Attribute Processor #1935

Closed

isgoutham changed the title ~~Added support for array values for attributes~~ Added support for list values for a key in processor Oct 12, 2020

bogdandrutu reviewed Oct 13, 2020

View reviewed changes

james-bebbington reviewed Oct 13, 2020

View reviewed changes

isgoutham added 5 commits October 13, 2020 11:26

Addressed review comments

297a709

removed map string interface case

8aecbbf

this is out of scope for this PR

Added testcases for AttributeValueArray

8022fef

Add more test cases

37b9c5e

added more coverage

afaa0a5

tigrannajaryan assigned bogdandrutu Oct 14, 2020

add tests

72616c4

isgoutham requested a review from a team October 20, 2020 08:11

fix lint issue

8a26330

update comment for TestSpan_Matching_Array

2e307bc

isgoutham requested review from bogdandrutu and james-bebbington October 20, 2020 12:26

tigrannajaryan reviewed Oct 26, 2020

View reviewed changes

tigrannajaryan requested changes Oct 27, 2020

View reviewed changes

tigrannajaryan closed this Oct 28, 2020

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Fix typo in packages.cmake causing incorrect nuget package versions (o…

52309dd

…pen-telemetry#1936)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for list values for a key in processor #1936

Added support for list values for a key in processor #1936

isgoutham commented Oct 12, 2020

linux-foundation-easycla bot commented Oct 12, 2020 •

edited

Loading

isgoutham Oct 12, 2020

bogdandrutu Oct 13, 2020

isgoutham Oct 13, 2020 •

edited

Loading

codecov bot commented Oct 12, 2020 •

edited

Loading

bogdandrutu Oct 13, 2020

isgoutham Oct 13, 2020

bogdandrutu Oct 13, 2020

isgoutham Oct 13, 2020

james-bebbington Oct 13, 2020 •

edited

Loading

isgoutham Oct 13, 2020 •

edited

Loading

isgoutham Oct 13, 2020

isgoutham commented Oct 20, 2020

isgoutham commented Oct 22, 2020

asldevi commented Oct 23, 2020

tigrannajaryan Oct 26, 2020

isgoutham Oct 27, 2020 •

edited

Loading

tigrannajaryan left a comment

tigrannajaryan commented Oct 28, 2020

Added support for list values for a key in processor #1936

Added support for list values for a key in processor #1936

Conversation

isgoutham commented Oct 12, 2020

linux-foundation-easycla bot commented Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isgoutham Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-bebbington Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

isgoutham Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isgoutham commented Oct 20, 2020

isgoutham commented Oct 22, 2020

asldevi commented Oct 23, 2020

Choose a reason for hiding this comment

isgoutham Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

tigrannajaryan left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Oct 28, 2020

linux-foundation-easycla bot commented Oct 12, 2020 •

edited

Loading

isgoutham Oct 13, 2020 •

edited

Loading

codecov bot commented Oct 12, 2020 •

edited

Loading

james-bebbington Oct 13, 2020 •

edited

Loading

isgoutham Oct 13, 2020 •

edited

Loading

isgoutham Oct 27, 2020 •

edited

Loading