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

feat: Add span adjuster that moves some OTel Resource attributes to Span.Process #4844

Merged
merged 7 commits into from
Oct 16, 2023
Merged

feat: Add span adjuster that moves some OTel Resource attributes to Span.Process #4844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0caee8d
Fix OTEL missing and wrong place process tags
james-ryans Oct 15, 2023
214c268
Add License to newly added files
james-ryans Oct 15, 2023
2546861
Optimize OTel tag adjuster performance
james-ryans Oct 15, 2023
392b38a
Remove HostID resource detector from HotROD tracer provider
james-ryans Oct 15, 2023
a50c173
Add coverage for shifting tags element to the left
james-ryans Oct 15, 2023
38325a4
Fix OTelTagAdjuster with otel library tags unit test expected value
james-ryans Oct 16, 2023
1d78dab
Ensure span.Tags slice has not been re-allocated
james-ryans Oct 16, 2023
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
1 change: 1 addition & 0 deletions cmd/query/app/querysvc/adjusters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func StandardAdjusters(maxClockSkewAdjust time.Duration) []adjuster.Adjuster {
adjuster.SpanIDDeduper(),
adjuster.ClockSkew(maxClockSkewAdjust),
adjuster.IPTagAdjuster(),
adjuster.OTelTagAdjuster(),
adjuster.SortLogFields(),
adjuster.SpanReferences(),
adjuster.ParentReference(),
Expand Down
2 changes: 1 addition & 1 deletion cmd/tracegen/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/resource"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
Expand Down
19 changes: 14 additions & 5 deletions examples/hotrod/pkg/tracing/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/resource"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

Expand Down Expand Up @@ -60,13 +60,22 @@ func InitOTEL(serviceName string, exporterType string, metricsFactory metrics.Fa

rpcmetricsObserver := rpcmetrics.NewObserver(metricsFactory, rpcmetrics.DefaultNameNormalizer)

res, err := resource.New(
context.Background(),
resource.WithSchemaURL(semconv.SchemaURL),
resource.WithAttributes(semconv.ServiceNameKey.String(serviceName)),
resource.WithTelemetrySDK(),
resource.WithHost(),
resource.WithOSType(),
)
if err != nil {
logger.Bg().Fatal("resource creation failed", zap.Error(err))
}

tp := sdktrace.NewTracerProvider(
sdktrace.WithBatcher(exp, sdktrace.WithBatchTimeout(1000*time.Millisecond)),
sdktrace.WithSpanProcessor(rpcmetricsObserver),
sdktrace.WithResource(resource.NewWithAttributes(
semconv.SchemaURL,
semconv.ServiceNameKey.String(serviceName),
)),
sdktrace.WithResource(res),
)
logger.Bg().Debug("Created OTEL tracer", zap.String("service-name", serviceName))
return tp
Expand Down
2 changes: 1 addition & 1 deletion examples/hotrod/pkg/tracing/rpcmetrics/observer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"

"github.com/jaegertracing/jaeger/pkg/metrics"
Expand Down
2 changes: 1 addition & 1 deletion examples/hotrod/pkg/tracing/rpcmetrics/observer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
"go.opentelemetry.io/otel/codes"
"go.opentelemetry.io/otel/sdk/resource"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"

u "github.com/jaegertracing/jaeger/internal/metricstest"
Expand Down
2 changes: 1 addition & 1 deletion examples/hotrod/services/customer/database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"fmt"

"go.opentelemetry.io/otel/attribute"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

Expand Down
2 changes: 1 addition & 1 deletion jaeger-ui
Open in desktop
Submodule jaeger-ui updated 162 files
51 changes: 51 additions & 0 deletions model/adjuster/otel_tag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright (c) 2023 The Jaeger Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package adjuster

import (
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"

"github.com/jaegertracing/jaeger/model"
)

var otelLibraryKeys = map[string]struct{}{
string(semconv.OTelLibraryNameKey): {},
string(semconv.OTelLibraryVersionKey): {},
}

func OTelTagAdjuster() Adjuster {
adjustSpanTags := func(span *model.Span) {
newI := 0
for i, tag := range span.Tags {
if _, ok := otelLibraryKeys[tag.Key]; ok {
span.Process.Tags = append(span.Process.Tags, tag)
continue
}
if i != newI {
span.Tags[newI] = tag
}
newI++
}
span.Tags = span.Tags[:newI]
}

return Func(func(trace *model.Trace) (*model.Trace, error) {
for _, span := range trace.Spans {
adjustSpanTags(span)
model.KeyValues(span.Process.Tags).Sort()
}
return trace, nil
})
}
95 changes: 95 additions & 0 deletions model/adjuster/otel_tag_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Copyright (c) 2023 The Jaeger Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package adjuster

import (
"testing"

"github.com/stretchr/testify/assert"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"

"github.com/jaegertracing/jaeger/model"
)

func TestOTelTagAdjuster(t *testing.T) {
testCases := []struct {
description string
span *model.Span
expected *model.Span
}{
{
description: "span with otel library tags",
span: &model.Span{
Tags: model.KeyValues{
model.String("random_key", "random_value"),
model.String(string(semconv.OTelLibraryNameKey), "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"),
model.String(string(semconv.OTelLibraryVersionKey), "0.45.0"),
model.String("another_key", "another_value"),
},
james-ryans marked this conversation as resolved.
Show resolved Hide resolved
Process: &model.Process{
Tags: model.KeyValues{},
},
},
expected: &model.Span{
Tags: model.KeyValues{
model.String("random_key", "random_value"),
model.String("another_key", "another_value"),
},
Process: &model.Process{
Tags: model.KeyValues{
model.String(string(semconv.OTelLibraryNameKey), "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"),
model.String(string(semconv.OTelLibraryVersionKey), "0.45.0"),
},
},
},
},
{
description: "span without otel library tags",
span: &model.Span{
Tags: model.KeyValues{
model.String("random_key", "random_value"),
},
Process: &model.Process{
Tags: model.KeyValues{},
},
},
expected: &model.Span{
Tags: model.KeyValues{
model.String("random_key", "random_value"),
},
Process: &model.Process{
Tags: model.KeyValues{},
},
},
},
}
for _, testCase := range testCases {
t.Run(testCase.description, func(t *testing.T) {
beforeTags := testCase.span.Tags

trace := &model.Trace{
Spans: []*model.Span{testCase.span},
}
trace, err := OTelTagAdjuster().Adjust(trace)
assert.NoError(t, err)
assert.Equal(t, testCase.expected.Tags, trace.Spans[0].Tags)
assert.Equal(t, testCase.expected.Process.Tags, trace.Spans[0].Process.Tags)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding this sort of test to ensure that Span.tags slice has not been re-allocated: https://go.dev/play/p/euEi6j19DXN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. do you have in mind on how to test this? 🤔 I actually can't find out how to ensure this, or can you give me an actual test case?

Copy link
Member

@yurishkuro yurishkuro Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Pretty much like in the example I pointed to. Before L83 store a reference to beforeTags := testCase.span.Tags slice. After Adjust is called:

newTag := model.String("new tag", "new value")
beforeTags[0] = newTag
assert.Equal(t, newTag, testCase.span.Tags[0], "span.Tags still points to the same underlying array")

The assert confirms that beforeTags and testCase.span.Tags still point to the same underlying slice.
You can even add

assert.NotEqual(t, beforeTags, testCase.span.Tags)

Copy link
Contributor Author

@james-ryans james-ryans Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, now I get what you meant. I've added the test, but I don't think assert.NotEqual(t, beforeTags, testCase.span.Tags) works since beforeTags and testCase.span.Tags point to the same underlying slice?

Copy link
Member

Choose a reason for hiding this comment

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

they point to the same underlying array, but the slices are different - the one after the adjustment is shorter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh right..


newTag := model.String("new_key", "new_value")
beforeTags[0] = newTag
assert.Equal(t, newTag, testCase.span.Tags[0], "span.Tags still points to the same underlying array")
})
}
}
2 changes: 1 addition & 1 deletion pkg/jtracer/jtracer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
"go.opentelemetry.io/otel/propagation"
"go.opentelemetry.io/otel/sdk/resource"
sdktrace "go.opentelemetry.io/otel/sdk/trace"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"
)

Expand Down
2 changes: 1 addition & 1 deletion plugin/metrics/prometheus/metricsstore/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import (
promapi "github.com/prometheus/client_golang/api/prometheus/v1"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

Expand Down
2 changes: 1 addition & 1 deletion plugin/storage/cassandra/spanstore/reader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (

"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/codes"
semconv "go.opentelemetry.io/otel/semconv/v1.20.0"
semconv "go.opentelemetry.io/otel/semconv/v1.21.0"
"go.opentelemetry.io/otel/trace"
"go.uber.org/zap"

Expand Down