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

james-ryans · 2023-10-15T17:49:42Z

Which problem is this PR solving?

Resolves [OTEL] Process info is missing from the spans #4534

Description of the changes

Apply OTel resource discovery(detectors) since OTel has plenty of detectors under the hood. Link https://pkg.go.dev/go.opentelemetry.io/otel/[email protected]/resource
Add OTel tags to process tags adjuster and we need to adjust only otel.libray.name and otel.library.version. Found here https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/adf5bb501bc65d24ae48088bde37f3c0b13353d0/pkg/translator/jaeger/traces_to_jaegerproto.go#L172
Bump all OTel semconv to v1.21.0 since telemetrySDK detector use v1.21.0 semconv.

How was this change tested?

Added a new unit test for the adjuster
Tested manual in local by running set OTEL_EXPORTER_OTLP_ENDPOINT http://localhost:4318; go run main.go all --otel-exporter=otlp and make run-all-in-one, and ordered a ride in HotROD app.

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: James Ryans <[email protected]>

codecov · 2023-10-15T17:57:14Z

Codecov Report

All modified lines are covered by tests ✅

Files	Coverage Δ
cmd/query/app/querysvc/adjusters.go	`100.00% <100.00%> (ø)`
examples/hotrod/pkg/tracing/rpcmetrics/observer.go	`70.00% <ø> (ø)`
model/adjuster/otel_tag.go	`100.00% <100.00%> (ø)`
plugin/metrics/prometheus/metricsstore/reader.go	`100.00% <ø> (ø)`
plugin/storage/cassandra/spanstore/reader.go	`100.00% <ø> (ø)`

📢 Thoughts on this report? Let us know!.

yurishkuro · 2023-10-15T18:08:00Z

model/adjuster/otel_tag.go

+}
+
+func OTelTagAdjuster() Adjuster {
+	return Func(func(trace *model.Trace) (*model.Trace, error) {


nit: please move to a helper function to reduce nesting (you have 5 levels of indentation)

moved it to an anonymous function

yurishkuro · 2023-10-15T18:12:48Z

model/adjuster/otel_tag.go

+func OTelTagAdjuster() Adjuster {
+	return Func(func(trace *model.Trace) (*model.Trace, error) {
+		for _, span := range trace.Spans {
+			filteredTags := make([]model.KeyValue, 0)


I want to be mindful of allocations & perf overhead. I liked this in-place approach better: https://github.com/jaegertracing/jaeger/pull/4841/files#diff-ca36bdd0bff9f5671449689223699d0029147f11212f797269850af1585aab21R46

optimized it

yurishkuro

lgtm

Signed-off-by: James Ryans <[email protected]>

yurishkuro · 2023-10-15T18:35:48Z

is this change breaking HotROD somehow? The integration test failed several times

james-ryans · 2023-10-15T18:41:30Z

is this change breaking HotROD somehow? The integration test failed several times

Looking into it.. it works normally in my local

Signed-off-by: James Ryans <[email protected]>

james-ryans · 2023-10-15T19:23:41Z

Okay, so I've ran the example/hotrod-integration-test.sh locally and logged the example-hotrod container, turns out that the resource.WithHostID() caused the error since the HotROD docker container was built using distroless base image but the resourrce.WithHostID() trying to read from /etc/machine-id or /var/lib/dbus/machine-id file.

2023-10-15T19:01:09.599Z	INFO	[email protected]/command.go:970	Using Prometheus as metrics backend
2023-10-15T19:01:09.599Z	INFO	[email protected]/command.go:940	Starting all services
2023-10-15T19:01:09.600Z	FATAL	tracing/init.go:73	resource creation failed	{"service": "customer", "error": "1 errors occurred detecting resource:\n\t* host id not found in: /etc/machine-id or /var/lib/dbus/machine-id"}
github.com/jaegertracing/jaeger/examples/hotrod/pkg/tracing.InitOTEL
	github.com/jaegertracing/jaeger/examples/hotrod/pkg/tracing/init.go:73
github.com/jaegertracing/jaeger/examples/hotrod/services/customer.NewServer
	github.com/jaegertracing/jaeger/examples/hotrod/services/customer/server.go:44
github.com/jaegertracing/jaeger/examples/hotrod/cmd.glob..func2
	github.com/jaegertracing/jaeger/examples/hotrod/cmd/customer.go:37

So, for this example, I'll just remove the resource.HostID() detector which the traces will not have host.id process tags.

james-ryans · 2023-10-15T19:39:21Z

Actually, I haven't added the OTel builtin detectors to jaeger components, should I add them? If yes, can you tell me which detector is needed?

yurishkuro · 2023-10-15T20:04:24Z

ran the example/hotrod-integration-test.sh locally and logged the example-hotrod container

Yeah, this should've been automated -- #4845

Actually, I haven't added the OTel builtin detectors to jaeger components, should I add them? If yes, can you tell me which detector is needed?

Same ones are fine, but let's do it in a separate PR

model/adjuster/otel_tag_test.go

yurishkuro · 2023-10-15T19:51:01Z

model/adjuster/otel_tag_test.go

+			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)


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

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?

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)

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?

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

Ohh right..

## Which problem is this PR solving? - The HotROD CIT was failing in #4844, but no logs were shown to understand why ## Description of the changes - Run container with `--name example-hotrod` - Add `docker logs example-hotrod` to CIT workflow in case of failure Signed-off-by: Yuri Shkuro <[email protected]>

Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: James Ryans <[email protected]>

Signed-off-by: James Ryans <[email protected]>

## Which problem is this PR solving? - Resolves #4534 - Continuation from #4844 ## Description of the changes - Add OTel resources to Jaeger components and tracegen tool. ## How was this change tested? - Tested manually in local <img width="1440" alt="image" src="https://github.com/jaegertracing/jaeger/assets/46216691/28bfb121-0c5a-42b1-a3e3-2c5915c90b17"> <img width="1440" alt="image" src="https://github.com/jaegertracing/jaeger/assets/46216691/3b6d993a-2fac-4c65-97c2-e3dca581a3a1"> ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: James Ryans <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]>

james-ryans added 2 commits October 16, 2023 00:24

Fix OTEL missing and wrong place process tags

0caee8d

Signed-off-by: James Ryans <[email protected]>

Add License to newly added files

214c268

Signed-off-by: James Ryans <[email protected]>

james-ryans requested a review from a team as a code owner October 15, 2023 17:49

james-ryans requested a review from yurishkuro October 15, 2023 17:49

yurishkuro reviewed Oct 15, 2023

View reviewed changes

yurishkuro changed the title ~~Fix OTEL missing and wrong place process tags~~ feat: Add span adjuster that moves some OTel Resource attributes to Span.Process Oct 15, 2023

Optimize OTel tag adjuster performance

2546861

Signed-off-by: James Ryans <[email protected]>

yurishkuro previously approved these changes Oct 15, 2023

View reviewed changes

Remove HostID resource detector from HotROD tracer provider

392b38a

Signed-off-by: James Ryans <[email protected]>

james-ryans dismissed yurishkuro’s stale review via 392b38a October 15, 2023 19:11

yurishkuro mentioned this pull request Oct 15, 2023

ci(hotrod): Print HotROD container logs in case of test failure #4845

Merged

yurishkuro reviewed Oct 15, 2023

View reviewed changes

james-ryans and others added 3 commits October 16, 2023 06:56

Add coverage for shifting tags element to the left

a50c173

Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: James Ryans <[email protected]>

Fix OTelTagAdjuster with otel library tags unit test expected value

38325a4

Signed-off-by: James Ryans <[email protected]>

Ensure span.Tags slice has not been re-allocated

1d78dab

Signed-off-by: James Ryans <[email protected]>

yurishkuro approved these changes Oct 16, 2023

View reviewed changes

yurishkuro merged commit c0e4ff1 into jaegertracing:main Oct 16, 2023
31 checks passed

dtsulik mentioned this pull request Oct 16, 2023

[WIP] feat: implement adjuster moving tags from span to process #4841

Closed

4 tasks

yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Oct 17, 2023

james-ryans mentioned this pull request Oct 19, 2023

Add OTel resource detector to Jaeger components #4864

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

james-ryans commented Oct 15, 2023 •

edited by yurishkuro

Loading

codecov bot commented Oct 15, 2023 •

edited

Loading

yurishkuro Oct 15, 2023

james-ryans Oct 15, 2023

yurishkuro Oct 15, 2023

james-ryans Oct 15, 2023

yurishkuro left a comment

yurishkuro commented Oct 15, 2023

james-ryans commented Oct 15, 2023

james-ryans commented Oct 15, 2023

james-ryans commented Oct 15, 2023

yurishkuro commented Oct 15, 2023

yurishkuro Oct 15, 2023

james-ryans Oct 16, 2023

yurishkuro Oct 16, 2023 •

edited

Loading

james-ryans Oct 16, 2023 •

edited

Loading

yurishkuro Oct 16, 2023

james-ryans Oct 16, 2023

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

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

Conversation

james-ryans commented Oct 15, 2023 • edited by yurishkuro Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Oct 15, 2023 • 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

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro commented Oct 15, 2023

james-ryans commented Oct 15, 2023

james-ryans commented Oct 15, 2023

james-ryans commented Oct 15, 2023

yurishkuro commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

james-ryans Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-ryans commented Oct 15, 2023 •

edited by yurishkuro

Loading

codecov bot commented Oct 15, 2023 •

edited

Loading

yurishkuro Oct 16, 2023 •

edited

Loading

james-ryans Oct 16, 2023 •

edited

Loading