prevent NPE when capturing headers

[SylvainJuge]

What does this PR do?

Fixes #1840

Checklist

• This is a bugfix
  • I have updated CHANGELOG.asciidoc
  • I have added tests that would fail without this fix

For reviewer:

• Do we have any usage of null header names or keys in practice ? While the underlying maps may allow for null values, I think that it would not make any sense to try capturing those.

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #1842 updated

• Start Time: 2021-06-08T08:17:19.529+0000

• Duration: 57 min 31 sec

• Commit: 55c1c5e

Test stats 🧪

Test	Results
Failed	0
Passed	2193
Skipped	18
Total	2211

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	2193
Skipped	18
Total	2211

[eyalkoren]

Do we have any usage of null header names or keys in practice ? While the underlying maps may allow for null values, I think that it would not make any sense to try capturing those.

This code path is adding headers to span context by iterating over actual existing record headers, so I assume we should capture them as is in order to reflect metadata about the record.
null keys are not allowed in the docs, so we can guard from null even though such are not expected (because they come from the sent record, so they already passed this screening).

null values are allowed, so if the user uses them, they may have a meaning, in which case it worth reporting those.
I think the easier fix is to add "null" to the value buffer in BinaryHeaderMap#add() if the value is null.

BTW, in NoRandomAccessMap#add() (the text header map counterpart), null values are allowed and are added to the map, however they are omitted during serialization. Either way we decide, the behavior or text header and binary headers should be aligned.

[SylvainJuge]

Good point, here is my revised proposal:

• allow for null values for both binary and text headers
• serialize them as null instead of omitting them, which would be consistent with what we do for HTTP headers

[eyalkoren]

The reason I suggested to serialize to "null" string is the way the binary headers are implemented. You can also store the bytes of this string and identify it in the iterator and return null as the value if you prefer, only make sure that the intake API allows for null values for headers.

[SylvainJuge]

Both the HTTP and message headers allow for null in the protocol schema for headers:

            "headers": {
              "description": "Headers received with the message, similar to HTTP request headers.",
              "type": [
                "null",
                "object"
              ],
              "additionalProperties": false,
              "patternProperties": {
                "[.*]*$": {
                  "type": [
                    "null",
                    "array",
                    "string"
                  ],
                  "items": {
                    "type": "string"
                  }
                }
              }
            },

[eyalkoren]

💡

[eyalkoren]

Make sure to enlargeValueLengths() if required to avoid ArrayIndexOutOfBoundsException.
Better yet, extract this code into a method to be reused with the same done below, something like:

int size = keys.size(); 
keys.add(key); 
if (size == valueLengths.length) {
  enlargeValueLengths();
}
valueLengths[size] = length;

[SylvainJuge]

now properly fixed & tested.

[eyalkoren]

Thanks! ❤️