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.

Sign up for GitHub

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

Jump to bottom

Drop invalid array elements in OTLP payload #342

Closed
wants to merge 3 commits into from
Closed

Drop invalid array elements in OTLP payload #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d03a251
drop invalid array elements in otlp payload
rubvs Aug 13, 2024
d71a8df
replace switch with if-statement
rubvs Aug 14, 2024
f1c92a2
add unit test to translate span
rubvs Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions input/otlp/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,15 +536,23 @@ func setLabel(key string, event *modelpb.APMEvent, v interface{}) {
}
switch v[0].(type) {
case string:
value := make([]string, len(v))
value := make([]string, 0, len(v))
for i := range v {
value[i] = v[i].(string)
// Ensure that the array is homogeneous
// Drop value that is not OTEL supported: https://opentelemetry.io/docs/specs/otel/common/
if r, ok := v[i].(string); ok {
value = append(value, r)
}
}
modelpb.Labels(event.Labels).SetSlice(key, value)
case float64:
value := make([]float64, len(v))
value := make([]float64, 0, len(v))
for i := range v {
value[i] = v[i].(float64)
// Ensure that the array is homogeneous
// Drop value that is not OTEL supported: https://opentelemetry.io/docs/specs/otel/common/
if r, ok := v[i].(float64); ok {
value = append(value, r)
}
}
modelpb.NumericLabels(event.NumericLabels).SetSlice(key, value)
}
Expand Down
70 changes: 70 additions & 0 deletions input/otlp/metadata_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@ import (
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/assert"
"go.opentelemetry.io/collector/pdata/pcommon"
"go.opentelemetry.io/collector/pdata/ptrace"
"google.golang.org/protobuf/testing/protocmp"

"github.com/elastic/apm-data/input/otlp"
"github.com/elastic/apm-data/model/modelpb"
)

Expand Down Expand Up @@ -342,6 +344,74 @@ func TestResourceLabels(t *testing.T) {
}, modelpb.NumericLabels(metadata.NumericLabels))
}

func TestTranslateSpanValue(t *testing.T) {

m := pcommon.NewMap()
m.PutStr("s", "v")
m.PutBool("b", true)
m.PutInt("i", 1.0)
m.PutDouble("f", 1.0)

e := &modelpb.APMEvent{
Event: &modelpb.Event{},
Span: &modelpb.Span{},
Labels: make(map[string]*modelpb.LabelValue),
NumericLabels: make(map[string]*modelpb.NumericLabelValue),
}

otlp.TranslateSpan(ptrace.SpanKindInternal, m, e)

assert.Equal(t, "v", e.GetLabels()["s"].Value)
assert.Equal(t, "true", e.GetLabels()["b"].Value)
assert.Equal(t, float64(1), e.GetNumericLabels()["i"].Value)
assert.Equal(t, float64(1), e.GetNumericLabels()["f"].Value)
}

func TestTranslateSpanSlice(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

If I look at this test without the context that this actually tests setLabel, I'd wonder why we test TranslateSpan only, but not TranslateTransaction. I think it makes sense to add a dimension to this table driven test so that it tests both TranslateSpan and TranslateTransaction. Span events also use setLabel, but convertSpanEvent is slightly complex, so I'm fine with leaving that out unless others feel strongly about it.


rubvs marked this conversation as resolved.
Show resolved Hide resolved
testCases := []struct {
desc string
input []any
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
input []any
input func() pcommon.Map

Would it be possible to consolidate TestTranslateSpanValue into this test by having input as a function that returns a map?

expected int
Copy link
Member

Choose a reason for hiding this comment

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

Better to assert both expected label value and expected numeric label value instead of just checking the length. There will be minimal increase to the line of code, but it will provide real value in checking if the elements are really correct. Also, if the test really fails, the assertion output will be less useful when it just compares length.

}{
{
desc: "drop non-string elements",
input: []any{"a", 1, 2},
expected: 1,
},
{
desc: "drop non-float64 elements",
input: []any{1.1, "a", "b"},
expected: 1,
},
{
desc: "drop all elements",
input: []any{nil, nil, nil},
expected: 0,
},
}
for _, tC := range testCases {
t.Run(tC.desc, func(t *testing.T) {

m := pcommon.NewMap()
s := m.PutEmptySlice("k")
s.FromRaw(tC.input)

e := &modelpb.APMEvent{
Event: &modelpb.Event{},
Span: &modelpb.Span{},
Labels: make(map[string]*modelpb.LabelValue),
NumericLabels: make(map[string]*modelpb.NumericLabelValue),
}

otlp.TranslateSpan(ptrace.SpanKindInternal, m, e)

actual := len(e.Labels) + len(e.NumericLabels)
assert.Equal(t, tC.expected, actual)
})
}
}

func transformResourceMetadata(t *testing.T, resourceAttrs map[string]interface{}) *modelpb.APMEvent {
traces, spans := newTracesSpans()
traces.ResourceSpans().At(0).Resource().Attributes().FromRaw(resourceAttrs)
Expand Down