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.

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

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

Merged

Conversation

frzifus
Copy link
Member

@frzifus frzifus commented Jul 27, 2022

Signed-off-by: Benedikt Bongartz bongartz@klimlive.de

Description:

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

Link to tracking Issue:

Testing:

  • Updated existing unit tests

Documentation:

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

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

@github-actions github-actions bot closed this Sep 8, 2022
@frzifus
Copy link
Member Author

frzifus commented Sep 16, 2022

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

could you help @jpkrohling ?

@jpkrohling jpkrohling reopened this Sep 16, 2022
@github-actions github-actions bot removed the Stale label Sep 17, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2022

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

@github-actions github-actions bot added the Stale label Oct 1, 2022
@dmitryax dmitryax removed the Stale label Oct 7, 2022
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

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

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 frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from 128b17c to 7d182c9 Compare October 11, 2022 18:29
@frzifus
Copy link
Member Author

frzifus commented Oct 11, 2022

ups seems my rebase messed things up...

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

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

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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.

@dmitryax
Copy link
Member

Please add a changelog entry

attrs.Remove(tracetranslator.TagError)
statusExists = true
}
attrs.Remove(tracetranslator.TagError)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

the changes got reverted.

tracetranslator.TagError: true,
conventions.AttributeHTTPStatusCode: 404,
},
status: errorStatus,
Copy link
Member

Choose a reason for hiding this comment

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

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.

@frzifus frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from c922e9c to 684f8ba Compare October 19, 2022 16:32
@frzifus 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 frzifus marked this pull request as ready for review October 19, 2022 16:33
@frzifus frzifus requested review from a team October 19, 2022 16:33
…n case of server kind

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
@frzifus frzifus force-pushed the fix/dont_overwrite_status_status_#8273 branch from 684f8ba to 0c992c1 Compare October 19, 2022 16:36
@frzifus
Copy link
Member Author

frzifus commented Oct 19, 2022

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.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I believe this is correct now.

@jpkrohling
Copy link
Member

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

@dmitryax dmitryax merged commit 367a1bb into open-telemetry:main Nov 8, 2022
@frzifus frzifus deleted the fix/dont_overwrite_status_status_#8273 branch November 8, 2022 19:12
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
…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 <bongartz@klimlive.de>
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jaeger span is in error if tag error=false AND http.status_code >=400
4 participants