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

Empty headers injected by HTTP and Baggage propagator #1366

Closed
yzsolt opened this issue May 2, 2022 · 3 comments · Fixed by #1367 or #1373
Closed

Empty headers injected by HTTP and Baggage propagator #1366

yzsolt opened this issue May 2, 2022 · 3 comments · Fixed by #1367 or #1373
Assignees

Comments

@yzsolt
Copy link
Contributor

yzsolt commented May 2, 2022

If the HTTP propagator is used on an invalid context or the baggage propagator is used on an empty baggage, the resulting header map will contain empty values. I don't know if this is intended or not, but I think it'd be more user friendly if empty values wouldn't be injected, so the user wouldn't have to remove or check them manually before using them as HTTP request headers.

@lalitb
Copy link
Member

lalitb commented May 2, 2022

A quick look into the HTTP propagator code, I don't think empty header files are injected if the context is not valid as we return early in that case:

if (!span_context.IsValid())
{
return;
}

Let me know if I am missing something here.
Though this may be true for the Baggage propagator and need to be fixed if so.

@lalitb
Copy link
Member

lalitb commented May 3, 2022

@yzsolt #1367 should fix the issue with baggage propagator. Can you check HTTP propagator once again, as our tests does validate that HTTP headers are not populated in case the context is invalid.

@yzsolt
Copy link
Contributor Author

yzsolt commented May 4, 2022

@lalitb Sorry for necro-bumping this issue, but I've finally had time to check it once again, and the Tracestate header still can be empty. I've also found the root cause: while there is a test case for this, it's checking the extracted carrier variable instead of the injected carrier2. So Tracestate header injection still needs an if and the tests should be fixed too. I'd suggest using more meaningful names for the extracted/injected carriers in all propagation tests to avoid an issue like this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants