Skip to content
New issue

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

#

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

Already on GitHub? # to your account

[v2][adjuster] Implement otel attribute adjuster to operate on otlp data model #6358

Merged
merged 15 commits into from
Dec 14, 2024
Merged

[v2][adjuster] Implement otel attribute adjuster to operate on otlp data model #6358

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a0ca95e
Implement OTEL Tag Adjuster To Operate on OTLP Data Model
mahadzaryab1 Dec 14, 2024
1aee1d1
Add Unit Tests
mahadzaryab1 Dec 14, 2024
e620684
Fix Linter
mahadzaryab1 Dec 14, 2024
aa6baa0
Merge branch 'main' into otel-tag-adjuster
mahadzaryab1 Dec 14, 2024
0c812d9
Add Warning If There Is a Conflicting Value
mahadzaryab1 Dec 14, 2024
1fe8893
Remove Continue Block
mahadzaryab1 Dec 14, 2024
adfb793
Remove From Span Attributes
mahadzaryab1 Dec 14, 2024
2462d69
Fix Linting
mahadzaryab1 Dec 14, 2024
fa4fc43
Fix Lint
mahadzaryab1 Dec 14, 2024
2b94c0d
Fix Unit Tests And Create Helper For Adding Warning
mahadzaryab1 Dec 14, 2024
c6e87f6
Add Unit Tests For addWarning Helper
mahadzaryab1 Dec 14, 2024
f13cd76
Run Formatter
mahadzaryab1 Dec 14, 2024
9ccc4f8
Address Feedback
mahadzaryab1 Dec 14, 2024
b92c7e6
Address Feedback
mahadzaryab1 Dec 14, 2024
3a2a8be
Merge branch 'main' into otel-tag-adjuster
mahadzaryab1 Dec 14, 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
67 changes: 67 additions & 0 deletions cmd/query/app/querysvc/adjuster/otelattribute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright (c) 2024 The Jaeger Authors.
// SPDX-License-Identifier: Apache-2.0

package adjuster

import (
"fmt"

"go.opentelemetry.io/collector/pdata/pcommon"
"go.opentelemetry.io/collector/pdata/ptrace"

"github.com/jaegertracing/jaeger/pkg/otelsemconv"
)

var otelLibraryKeys = map[string]struct{}{
string(otelsemconv.TelemetrySDKLanguageKey): {},
string(otelsemconv.TelemetrySDKNameKey): {},
string(otelsemconv.TelemetrySDKVersionKey): {},
string(otelsemconv.TelemetryDistroNameKey): {},
string(otelsemconv.TelemetryDistroVersionKey): {},
}

// OTELAttribute creates an adjuster that removes the OpenTelemetry library attributes
// from spans and adds them to the attributes of a resource.
func OTELAttribute() Adjuster {
return Func(func(traces ptrace.Traces) (ptrace.Traces, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro these adjusters are currently making modifications to the input traces - is that fine or do we want to make a copy of the traces before returning them?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine, the same happened with the v1 adjusters.

Copy link
Member

Choose a reason for hiding this comment

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

But we could reflect that in the interface by making it not return anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good - i can do that in a follow-up PR

adjuster := otelAttributeAdjuster{}
Copy link
Member

Choose a reason for hiding this comment

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

it feels rather convoluted, having a Func then a struct. Why not just return the struct from the public function and have it implement interface (and any helper methods) directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done - I'll make the same change to the other adjusters in a follow-up PR

resourceSpans := traces.ResourceSpans()
for i := 0; i < resourceSpans.Len(); i++ {
rs := resourceSpans.At(i)
resource := rs.Resource()
scopeSpans := rs.ScopeSpans()
for j := 0; j < scopeSpans.Len(); j++ {
ss := scopeSpans.At(j)
spans := ss.Spans()
for k := 0; k < spans.Len(); k++ {
span := spans.At(k)
adjuster.adjust(span, resource)
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this is incorrect. You could, in theory, have two spans under the same Resource which have different library attributes, for whatever reason, which would require us to clone the resource. It should never happen in practice if the instrumentation is not messed up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yurishkuro I see - do we want to fix that?

Copy link
Member

Choose a reason for hiding this comment

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

rather than implementing the actual splitting of Resource I would go with soft validation - before copying attribute to Resource check if it already exists and if the value is the same. If the value is different then don't copy and instead add a warning to the span.

}
}
}
return traces, nil
})
}

type otelAttributeAdjuster struct{}

func (otelAttributeAdjuster) adjust(span ptrace.Span, resource pcommon.Resource) {
replace := make(map[string]pcommon.Value)
span.Attributes().Range(func(k string, v pcommon.Value) bool {
if _, ok := otelLibraryKeys[k]; ok {
replace[k] = v
}
return true
})
for k, v := range replace {
if existing, ok := resource.Attributes().Get(k); ok {
if existing.AsRaw() != v.AsRaw() {
span.Attributes().PutStr(adjusterWarningAttribute, fmt.Sprintf("conflicting attribute values for %s", k))
continue
}
} else {
v.CopyTo(resource.Attributes().PutEmpty(k))
span.Attributes().Remove(k)
}
}
}
98 changes: 98 additions & 0 deletions cmd/query/app/querysvc/adjuster/otelattribute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// Copyright (c) 2024 The Jaeger Authors.
// SPDX-License-Identifier: Apache-2.0

package adjuster

import (
"testing"

"github.com/jaegertracing/jaeger/pkg/otelsemconv"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/collector/pdata/ptrace"
)

func TestOTELAttributeAdjuster(t *testing.T) {
tests := []struct {
name string
input ptrace.Traces
expected ptrace.Traces
}{
{
name: "span with otel library attributes",
input: func() ptrace.Traces {
traces := ptrace.NewTraces()
rs := traces.ResourceSpans().AppendEmpty()
span := rs.ScopeSpans().AppendEmpty().Spans().AppendEmpty()
span.Attributes().PutStr("random_key", "random_value")
span.Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Go")
span.Attributes().PutStr(string(otelsemconv.TelemetrySDKNameKey), "opentelemetry")
span.Attributes().PutStr(string(otelsemconv.TelemetrySDKVersionKey), "1.27.0")
span.Attributes().PutStr(string(otelsemconv.TelemetryDistroNameKey), "opentelemetry")
span.Attributes().PutStr(string(otelsemconv.TelemetryDistroVersionKey), "blah")
span.Attributes().PutStr("another_key", "another_value")
return traces
}(),
expected: func() ptrace.Traces {
traces := ptrace.NewTraces()
rs := traces.ResourceSpans().AppendEmpty()
span := rs.ScopeSpans().AppendEmpty().Spans().AppendEmpty()
span.Attributes().PutStr("random_key", "random_value")
span.Attributes().PutStr("another_key", "another_value")
rs.Resource().Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Go")
rs.Resource().Attributes().PutStr(string(otelsemconv.TelemetrySDKNameKey), "opentelemetry")
rs.Resource().Attributes().PutStr(string(otelsemconv.TelemetrySDKVersionKey), "1.27.0")
rs.Resource().Attributes().PutStr(string(otelsemconv.TelemetryDistroNameKey), "opentelemetry")
rs.Resource().Attributes().PutStr(string(otelsemconv.TelemetryDistroVersionKey), "blah")
return traces
}(),
},
{
name: "span without otel library attributes",
input: func() ptrace.Traces {
traces := ptrace.NewTraces()
rs := traces.ResourceSpans().AppendEmpty()
span := rs.ScopeSpans().AppendEmpty().Spans().AppendEmpty()
span.Attributes().PutStr("random_key", "random_value")
return traces
}(),
expected: func() ptrace.Traces {
traces := ptrace.NewTraces()
rs := traces.ResourceSpans().AppendEmpty()
span := rs.ScopeSpans().AppendEmpty().Spans().AppendEmpty()
span.Attributes().PutStr("random_key", "random_value")
return traces
}(),
},
{
name: "span with conflicting otel library attributes",
input: func() ptrace.Traces {
traces := ptrace.NewTraces()
rs := traces.ResourceSpans().AppendEmpty()
rs.Resource().Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Go")
span := rs.ScopeSpans().AppendEmpty().Spans().AppendEmpty()
span.Attributes().PutStr("random_key", "random_value")
span.Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Java")
return traces
}(),
expected: func() ptrace.Traces {
traces := ptrace.NewTraces()
rs := traces.ResourceSpans().AppendEmpty()
span := rs.ScopeSpans().AppendEmpty().Spans().AppendEmpty()
span.Attributes().PutStr("random_key", "random_value")
span.Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Java")
span.Attributes().PutStr("jaeger.adjuster.warning", "conflicting attribute values for telemetry.sdk.language")
rs.Resource().Attributes().PutStr(string(otelsemconv.TelemetrySDKLanguageKey), "Go")
return traces
}(),
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
adjuster := OTELAttribute()
result, err := adjuster.Adjust(test.input)
require.NoError(t, err)
assert.EqualValues(t, test.expected, result)
})
}
}
5 changes: 5 additions & 0 deletions cmd/query/app/querysvc/adjuster/warning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package adjuster

const (
adjusterWarningAttribute = "jaeger.adjuster.warning"
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
adjusterWarningAttribute = "jaeger.adjuster.warning"
warningsAttribute = "jaeger.internal.warnings"

we need this as a general mechanism for warnings, not specific to adjusters. The UI will only look in one place for these warnings.

BTW since we're adding this as Span attribute we need to alter otlp->model translation to look for this attribute and move it to the proper Span.Warnings place. I think we should find all instances of us calling OTLP<>model translations in the code and replace them with internal methods so that we always go through the same logic (i.e. call transformer from OTEL first and then run our additional transformers).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good - should this attribute constant still live in package adjuster?

Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added an item for the otlp->model change to the tracking issue to address in a follow-up PR

Copy link
Member

Choose a reason for hiding this comment

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

no it should not be in the adjuster, we need a neutral place across all modules. Maybe internal/model_v2

Copy link
Collaborator Author

@mahadzaryab1 mahadzaryab1 Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@yurishkuro do we want to introduce the name model for v2? it feels like model has been synonymous with v1

Copy link
Member

Choose a reason for hiding this comment

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

I know, I don't like it either. Any other ideas? jotlp?

)
Loading