[translator/jaeger] For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER #12753

frzifus · 2022-07-27T14:41:56Z

Signed-off-by: Benedikt Bongartz [email protected]

Description:

This pr prevents the status code from being overwritten if the x error active is set to false.

Link to tracking Issue:

Closes jaeger span is in error if tag error=false AND http.status_code >=400 #8273

Testing:

Updated existing unit tests

Documentation:

Followed specification/semantic_conventions/status

pkg/translator/jaeger/jaegerproto_to_traces_test.go

github-actions · 2022-08-23T05:36:13Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-09-08T05:38:43Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

frzifus · 2022-09-16T12:33:15Z

mh.. i did like to reopen this pr. But it seems ive no permission to do so.

could you help @jpkrohling ?

github-actions · 2022-10-01T05:37:58Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

dmitryax

@frzifus thanks for the fix. could you please respond to the comments and rebase?

pkg/translator/jaeger/jaegerproto_to_traces.go

frzifus · 2022-10-11T18:32:51Z

ups seems my rebase messed things up...

dmitryax · 2022-10-12T04:29:32Z

@frzifus it needs another rebase. Also please add a change log entry in .chloggen

pkg/translator/jaeger/jaegerproto_to_traces.go

dmitryax · 2022-10-12T22:21:01Z

pkg/translator/jaeger/jaegerproto_to_traces.go

+		// in case of SpanKind.SERVER and MUST be set to Error in case of SpanKind.CLIENT.
+		// For HTTP status codes in the 5xx range, as well as any other code the client
+		// failed to interpret, span status MUST be set to Error.
+		if code, err := getStatusCodeFromHTTPStatusAttr(httpCodeAttr); span.Kind() == ptrace.SpanKindClient && err == nil {


I don't think it's correct logic here. Now we don't set error if span kind is server even for 5xx, but according to the spec we should, right? You can pass span kind to getStatusCodeFromHTTPStatusAttr and use it as condition only for 4xx codes.

pkg/translator/jaeger/jaegerproto_to_traces.go

dmitryax · 2022-10-12T22:22:04Z

Please add a changelog entry

jpkrohling · 2022-10-17T15:21:20Z

pkg/translator/jaeger/jaegerproto_to_traces.go

-			attrs.Remove(tracetranslator.TagError)
-			statusExists = true
+		}
+		attrs.Remove(tracetranslator.TagError)


We should not remove an attribute that has been explicitly added before us. Perhaps this is a 200 but an error occurred (the application might not be quite following REST principles).

The spec is a bit ambiguous here, as it states that it "must be left unset", but in this case, it was already set by something else (perhaps manually by developers? or using the attributes processor?).

@jpkrohling So far we have been interchangeably treating jaeger error tag and OTel StatusCode in jaeger receiver/exporter with the exception that it worked only with error=true in the receiver due to the bug fixed in this PR. We set error jaeger tag back from the OTel StatusCode in the exporter

opentelemetry-collector-contrib/pkg/translator/jaeger/traces_to_jaegerproto.go

Line 192 in 7e904d8

statusCodeTag, statusCodeTagFound = getTagFromStatusCode(status.Code())

. If we leave the error tag as an additional OTel attribute, I believe it'll result into duplicated jaeger error tag in the exporter. The correctness tests will also likely fail.

I'd suggest to keep the existing behavior and remove the tag but submit a separate issue where we discuss if it needs to be removed. PR to that issue would need to be applied to both receiver and exporter. WDYT?

the changes got reverted.

pkg/translator/jaeger/jaegerproto_to_traces.go

jpkrohling · 2022-10-17T15:30:43Z

pkg/translator/jaeger/jaegerproto_to_traces_test.go

+				tracetranslator.TagError:            true,
+				conventions.AttributeHTTPStatusCode: 404,
+			},
+			status:           errorStatus,


I must have read the code wrongly, as I would have expected this test to have failed. Given this test passes, the behavior is validated. The only other case that needs to be tested (in another PR perhaps?) is to set the error tag if the status code is greater than 500 and no error tag is set yet.

…n case of server kind Signed-off-by: Benedikt Bongartz <[email protected]>

Signed-off-by: Benedikt Bongartz <[email protected]>

frzifus · 2022-10-19T16:43:15Z

Hi @dmitryax @jpkrohling, I have tried to address your comments. However, I am not sure if I have understood the conversions here correctly. Therefore, please review my changes carefully. 🙈. Would be very thankful for further feedback/guidance.

jpkrohling

I believe this is correct now.

jpkrohling · 2022-10-19T17:35:08Z

@dmitryax, would you like to take a final look, or can I merge it as is?

pkg/translator/jaeger/jaegerproto_to_traces.go

…http codes (open-telemetry#12753) For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER Signed-off-by: Benedikt Bongartz <[email protected]>

frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from 00ac6e1 to 128b17c Compare July 27, 2022 14:55

frzifus mentioned this pull request Jul 27, 2022

jaeger span is in error if tag error=false AND http.status_code >=400 #8273

Closed

jpkrohling self-requested a review August 1, 2022 15:04

jpkrohling reviewed Aug 1, 2022

View reviewed changes

pkg/translator/jaeger/jaegerproto_to_traces_test.go Outdated Show resolved Hide resolved

github-actions bot added the Stale label Aug 23, 2022

frzifus mentioned this pull request Aug 23, 2022

REQUEST: New membership for frzifus open-telemetry/community#1139

Closed

6 tasks

github-actions bot closed this Sep 8, 2022

jpkrohling reopened this Sep 16, 2022

github-actions bot removed the Stale label Sep 17, 2022

github-actions bot added the Stale label Oct 1, 2022

dmitryax removed the Stale label Oct 7, 2022

dmitryax reviewed Oct 10, 2022

View reviewed changes

pkg/translator/jaeger/jaegerproto_to_traces.go Outdated Show resolved Hide resolved

pkg/translator/jaeger/jaegerproto_to_traces.go Outdated Show resolved Hide resolved

frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from 128b17c to 7d182c9 Compare October 11, 2022 18:29

frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from 7d182c9 to dc7a7b6 Compare October 11, 2022 18:54

dmitryax reviewed Oct 12, 2022

View reviewed changes

pkg/translator/jaeger/jaegerproto_to_traces.go Show resolved Hide resolved

dmitryax reviewed Oct 12, 2022

View reviewed changes

jpkrohling reviewed Oct 17, 2022

View reviewed changes

frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from c922e9c to 684f8ba Compare October 19, 2022 16:32

frzifus changed the title ~~[translator/jaeger] fix wrong assigned status code if the error is set to false~~ [translator/jaeger] For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER Oct 19, 2022

frzifus marked this pull request as ready for review October 19, 2022 16:33

frzifus requested review from a team October 19, 2022 16:33

github-actions bot assigned jpkrohling Oct 19, 2022

frzifus added 2 commits October 19, 2022 18:36

[pkg/translator/jaeger] in 4xx range span status MUST be left unset i…

3a63e9f

…n case of server kind Signed-off-by: Benedikt Bongartz <[email protected]>

[pkg/translator/jaeger] provide changelog

0c992c1

Signed-off-by: Benedikt Bongartz <[email protected]>

frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from 684f8ba to 0c992c1 Compare October 19, 2022 16:36

jpkrohling approved these changes Oct 19, 2022

View reviewed changes

dmitryax reviewed Oct 20, 2022

View reviewed changes

pkg/translator/jaeger/jaegerproto_to_traces.go Show resolved Hide resolved

dmitryax approved these changes Nov 8, 2022

View reviewed changes

pkg/translator/jaeger/jaegerproto_to_traces.go Show resolved Hide resolved

dmitryax merged commit 367a1bb into open-telemetry:main Nov 8, 2022

frzifus deleted the fix/dont_overwrite_status_status_#8273 branch November 8, 2022 19:12

jpkrohling mentioned this pull request Mar 1, 2023

[jaeger/receiver] Fix span status translation #19171

Merged

mayank-rocketml mentioned this pull request Apr 20, 2023

[Bug]: SPM - Error rate % not showing 4xx client errors. jaegertracing/jaeger#4400

Closed

plantfansam mentioned this pull request Jul 21, 2023

disregard #24427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[translator/jaeger] For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER #12753

[translator/jaeger] For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER #12753

frzifus commented Jul 27, 2022 •

edited

Loading

github-actions bot commented Aug 23, 2022

github-actions bot commented Sep 8, 2022

frzifus commented Sep 16, 2022

github-actions bot commented Oct 1, 2022

dmitryax left a comment

frzifus commented Oct 11, 2022

dmitryax commented Oct 12, 2022

dmitryax Oct 12, 2022

dmitryax commented Oct 12, 2022

jpkrohling Oct 17, 2022

dmitryax Oct 17, 2022

frzifus Oct 19, 2022

jpkrohling Oct 17, 2022

frzifus commented Oct 19, 2022

jpkrohling left a comment

jpkrohling commented Oct 19, 2022

[translator/jaeger] For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER #12753

[translator/jaeger] For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER #12753

Conversation

frzifus commented Jul 27, 2022 • edited Loading

github-actions bot commented Aug 23, 2022

github-actions bot commented Sep 8, 2022

frzifus commented Sep 16, 2022

github-actions bot commented Oct 1, 2022

dmitryax left a comment

Choose a reason for hiding this comment

frzifus commented Oct 11, 2022

dmitryax commented Oct 12, 2022

dmitryax Oct 12, 2022

Choose a reason for hiding this comment

dmitryax commented Oct 12, 2022

jpkrohling Oct 17, 2022

Choose a reason for hiding this comment

dmitryax Oct 17, 2022

Choose a reason for hiding this comment

frzifus Oct 19, 2022

Choose a reason for hiding this comment

jpkrohling Oct 17, 2022

Choose a reason for hiding this comment

frzifus commented Oct 19, 2022

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Oct 19, 2022

frzifus commented Jul 27, 2022 •

edited

Loading