Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

Fix bug when traceparent is too small #2047

Closed
Closed
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ public List<String> fields() {
// TODO(bdrutu): Do we need to verify that version is hex and that for the version
// the length is the expected one?
checkArgument(
traceparent.charAt(TRACE_OPTION_OFFSET - 1) == TRACEPARENT_DELIMITER
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a unit test reproducing the issue #2038 and confirming it's fixed by this change?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't find a recurrence here, but anyway, there is something wrong with the logic of this code, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, I'm not familiar with this code.

I do agree that the expression seems to have a redundant duplicate condition, though removing the first check may lead to some other issue since when it's "false" the remaining conditions won't get executed.
So I suggest to add a test reproducing the issue this change is supposed to fix.

&& (traceparent.length() == TRACEPARENT_HEADER_SIZE
(traceparent.length() == TRACEPARENT_HEADER_SIZE
|| (traceparent.length() > TRACEPARENT_HEADER_SIZE
&& traceparent.charAt(TRACEPARENT_HEADER_SIZE) == TRACEPARENT_DELIMITER))
&& traceparent.charAt(SPAN_ID_OFFSET - 1) == TRACEPARENT_DELIMITER
Expand Down