fix: correlation context propagation extract for a single entry

[rubenvp8510]

Signed-off-by: Ruben Vargas [email protected]

Which problem is this PR solving?

• The extraction of the correlation context fails when only have one entry "key=value"

Short description of the changes

• Fixes that case, and make sure it fails and return no correlation context if the header format is wrong.

[codecov]

Codecov Report

Merging #1336 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1336   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files         145      145           
  Lines        4334     4335    +1     
  Branches      883      883           
=======================================
+ Hits         4079     4080    +1     
  Misses        255      255           

Impacted Files	Coverage Δ
...tion-context/propagation/HttpCorrelationContext.ts	98.03% <100.00%> (+0.03%)	⬆️

[obecny]

this loops checks only a first pair, so either the loop is not needed, or you should check all pairs and if none of them matched then you should return context .
Hmm, something else is wrong here too or I don't get it.

What if the pair in the middle doesn't match and the rest pairs match ? Should it continue or it should return the context even though the previous pairs were added to the correlationContext.

shouldn't this look like this

let found = false;
pairs.forEach(entry => {
  const keyPair = this._parsePairKeyValue(entry);	
  if (keyPair) {
    found = true;
    correlationContext[keyPair.key] = { value: keyPair.value };
  }
})
if (!found)  {
  return context;
}
return setCorrelationContext(context, correlationContext);

[rubenvp8510]

That is a good question, as it is now if there is an error in the header it returns the context without correlationContext, as you can see in this test: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/test/correlation-context/HttpCorrelationContext.test.ts#L154 , but may be we would like change this and continue parsing the other keys and return the well formed keys WDYT?

[obecny]

hmm so if I understand correctly only the line if (pairs.length == 1) return context; should be removed and the rest stays as it was ?

[rubenvp8510]

If we want to have a correlation context and only exclude malformed keys , I would say yes, and of change the tests here: https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-core/test/correlation-context/HttpCorrelationContext.test.ts#L154 to reflect that fact.

[obecny]

I would like to hear here someone else opinion on that too, if that is the way to go?, @open-telemetry/javascript-approvers

[dyladan]

I think we should skip malformed pairs and keep processing.

[rubenvp8510]

Hi, I did the changes to skip only malformed keys, also modified the test of failures to include that case.

[obecny]

lgtm, one comment

[dyladan]

LGTM thanks