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

mahadzaryab1 · 2024-12-14T02:05:04Z

Which problem is this PR solving?

Towards Implement adjusters to operate on OTLP data format #6344

Description of the changes

This PR implements the OTelTag adjuster to operate on the OTLP data model. In the OTLP model, tags are dubbed as attributes so the adjuster was renamed to OTELAttribute.

How was this change tested?

Added unit tests

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: npm run lint and npm run test

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-12-14T02:31:19Z

cmd/query/app/querysvc/adjuster/otelattribute.go

+				spans := ss.Spans()
+				for k := 0; k < spans.Len(); k++ {
+					span := spans.At(k)
+					adjuster.adjust(span, resource)


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.

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

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.

codecov · 2024-12-14T06:02:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.13%. Comparing base (c6fa1af) to head (3a2a8be).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6358   +/-   ##
=======================================
  Coverage   96.13%   96.13%           
=======================================
  Files         358      360    +2     
  Lines       20391    20429   +38     
=======================================
+ Hits        19602    19639   +37     
  Misses        603      603           
- Partials      186      187    +1

Flag	Coverage Δ
badger_v1	`8.98% <ø> (ø)`
badger_v2	`1.63% <ø> (ø)`
cassandra-4.x-v1-manual	`14.96% <ø> (ø)`
cassandra-4.x-v2-auto	`1.57% <ø> (ø)`
cassandra-4.x-v2-manual	`1.57% <ø> (ø)`
cassandra-5.x-v1-manual	`14.96% <ø> (ø)`
cassandra-5.x-v2-auto	`1.57% <ø> (ø)`
cassandra-5.x-v2-manual	`1.57% <ø> (ø)`
elasticsearch-6.x-v1	`18.64% <ø> (-0.01%)`	⬇️
elasticsearch-7.x-v1	`18.73% <ø> (ø)`
elasticsearch-8.x-v1	`18.89% <ø> (ø)`
elasticsearch-8.x-v2	`1.62% <ø> (ø)`
grpc_v1	`10.49% <ø> (ø)`
grpc_v2	`7.92% <ø> (ø)`
kafka-v1	`9.30% <ø> (ø)`
kafka-v2	`1.63% <ø> (ø)`
memory_v2	`1.62% <ø> (-0.01%)`	⬇️
opensearch-1.x-v1	`18.77% <ø> (ø)`
opensearch-2.x-v1	`18.76% <ø> (-0.01%)`	⬇️
opensearch-2.x-v2	`1.63% <ø> (+<0.01%)`	⬆️
tailsampling-processor	`0.45% <ø> (ø)`
unittests	`95.01% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 · 2024-12-14T18:36:53Z

cmd/query/app/querysvc/adjuster/otelattribute.go

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


@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?

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

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

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

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 · 2024-12-14T18:55:05Z

cmd/query/app/querysvc/adjuster/otelattribute.go

+		} else {
+			v.CopyTo(resource.Attributes().PutEmpty(k))
+		}
+		span.Attributes().Remove(k)


@yurishkuro we're always removing the attribute from the span - even if the value conflicts. Is that fine?

no, in that case I would keep it. The design goal we observe is (a) capture input data into storage with as little rewrites as possible to assist in debugging of bad instrumentation, (b) at query time adjust the data as needed but still keep the originals visible in some way (since looking directly in storage is much harder than looking in the UI).

cmd/query/app/querysvc/adjuster/otelattribute.go

yurishkuro · 2024-12-14T18:57:44Z

cmd/query/app/querysvc/adjuster/otelattribute.go

+// from spans and adds them to the attributes of a resource.
+func OTELAttribute() Adjuster {
+	return Func(func(traces ptrace.Traces) (ptrace.Traces, error) {
+		adjuster := otelAttributeAdjuster{}


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?

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

yurishkuro · 2024-12-14T18:59:37Z

cmd/query/app/querysvc/adjuster/otelattribute.go

+	for k, v := range replace {
+		if existing, ok := resource.Attributes().Get(k); ok {
+			if existing.AsRaw() != v.AsRaw() {
+				span.Attributes().PutStr(adjusterWarningAttribute, "conflicting attribute values for "+k)


we should have a shared util function (across all adjusters) for adding warnings to the span, and the type of the warning attribute should be []string, not a single string. It looks like you are overriding the same warnings attribute now.

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

cmd/query/app/querysvc/adjuster/otelattribute.go

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro · 2024-12-14T21:58:49Z

cmd/query/app/querysvc/adjuster/resourceattributes.go

+	for k, v := range replace {
+		existing, ok := resource.Attributes().Get(k)
+		if ok && existing.AsRaw() != v.AsRaw() {
+			addWarning(span, "conflicting attribute values for "+k)


Suggested change

addWarning(span, "conflicting attribute values for "+k)

addWarning(span, "conflicting values between Span and Resource for attribute "+k)

yurishkuro · 2024-12-14T22:02:13Z

cmd/query/app/querysvc/adjuster/warning.go

+)
+
+const (
+	adjusterWarningAttribute = "jaeger.adjuster.warning"


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

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

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

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

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

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

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added 3 commits December 13, 2024 20:58

Implement OTEL Tag Adjuster To Operate on OTLP Data Model

a0ca95e

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Unit Tests

1aee1d1

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Linter

e620684

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 added the changelog:bugfix-or-minor-feature label Dec 14, 2024

mahadzaryab1 requested a review from yurishkuro December 14, 2024 02:05

mahadzaryab1 requested a review from a team as a code owner December 14, 2024 02:05

Merge branch 'main' into otel-tag-adjuster

aa6baa0

dosubot bot added area/otel v2 labels Dec 14, 2024

yurishkuro reviewed Dec 14, 2024

View reviewed changes

yurishkuro approved these changes Dec 14, 2024

View reviewed changes

mahadzaryab1 mentioned this pull request Dec 14, 2024

Implement adjusters to operate on OTLP data format #6344

Closed

13 tasks

Add Warning If There Is a Conflicting Value

0c812d9

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 commented Dec 14, 2024

View reviewed changes

mahadzaryab1 added 4 commits December 14, 2024 13:37

Remove Continue Block

1fe8893

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Remove From Span Attributes

adfb793

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Linting

2462d69

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Fix Lint

fa4fc43

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 commented Dec 14, 2024

View reviewed changes

yurishkuro reviewed Dec 14, 2024

View reviewed changes

cmd/query/app/querysvc/adjuster/otelattribute.go Outdated Show resolved Hide resolved

yurishkuro reviewed Dec 14, 2024

View reviewed changes

mahadzaryab1 added 2 commits December 14, 2024 14:36

Fix Unit Tests And Create Helper For Adding Warning

2b94c0d

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Add Unit Tests For addWarning Helper

c6e87f6

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro December 14, 2024 19:42

Run Formatter

f13cd76

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

yurishkuro reviewed Dec 14, 2024

View reviewed changes

cmd/query/app/querysvc/adjuster/otelattribute.go Outdated Show resolved Hide resolved

yurishkuro reviewed Dec 14, 2024

View reviewed changes

cmd/query/app/querysvc/adjuster/otelattribute.go Outdated Show resolved Hide resolved

Address Feedback

9ccc4f8

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

mahadzaryab1 requested a review from yurishkuro December 14, 2024 21:35

yurishkuro reviewed Dec 14, 2024

View reviewed changes

mahadzaryab1 and others added 2 commits December 14, 2024 17:03

Address Feedback

b92c7e6

Signed-off-by: Mahad Zaryab <mahadzaryab1@gmail.com>

Merge branch 'main' into otel-tag-adjuster

3a2a8be

yurishkuro approved these changes Dec 14, 2024

View reviewed changes

yurishkuro merged commit 10cb6a9 into jaegertracing:main Dec 14, 2024
54 checks passed

mahadzaryab1 deleted the otel-tag-adjuster branch January 6, 2025 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mahadzaryab1 commented Dec 14, 2024

yurishkuro Dec 14, 2024

mahadzaryab1 Dec 14, 2024

yurishkuro Dec 14, 2024

codecov bot commented Dec 14, 2024 •

edited

Loading

mahadzaryab1 Dec 14, 2024

yurishkuro Dec 14, 2024

yurishkuro Dec 14, 2024

mahadzaryab1 Dec 14, 2024

mahadzaryab1 Dec 14, 2024

yurishkuro Dec 14, 2024

yurishkuro Dec 14, 2024

mahadzaryab1 Dec 14, 2024

yurishkuro Dec 14, 2024

mahadzaryab1 Dec 14, 2024

yurishkuro Dec 14, 2024

yurishkuro Dec 14, 2024

mahadzaryab1 Dec 14, 2024

mahadzaryab1 Dec 14, 2024 •

edited

Loading

yurishkuro Dec 14, 2024

mahadzaryab1 Dec 14, 2024 •

edited

Loading

yurishkuro Dec 14, 2024

	addWarning(span, "conflicting attribute values for "+k)
	addWarning(span, "conflicting values between Span and Resource for attribute "+k)

	adjusterWarningAttribute = "jaeger.adjuster.warning"
	warningsAttribute = "jaeger.internal.warnings"

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

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

Conversation

mahadzaryab1 commented Dec 14, 2024

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 14, 2024 • 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

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

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

Choose a reason for hiding this comment

mahadzaryab1 Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahadzaryab1 Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 14, 2024 •

edited

Loading

mahadzaryab1 Dec 14, 2024 •

edited

Loading

mahadzaryab1 Dec 14, 2024 •

edited

Loading