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

Allow w3c to be set as the default propagation for opentelemetry plugin #10246

Closed
1 task done
nbialostosky opened this issue Feb 6, 2023 · 3 comments · Fixed by #10620
Closed
1 task done

Allow w3c to be set as the default propagation for opentelemetry plugin #10246

nbialostosky opened this issue Feb 6, 2023 · 3 comments · Fixed by #10620

Comments

@nbialostosky
Copy link
Contributor

nbialostosky commented Feb 6, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Kong version ($ kong version)

3.1.1

Current Behavior

With the current implementation of opentelemetry propagation, we are having issues with non-w3c trace id's being 8 byte once they get to our backend.

The Zipkin plugin allows you to set a default_header_type, though still has the similar issue with not converting 8byte to 16byte -- which we addressed with some changes locally.

Expected Behavior

Implement similar behavior as the Zipkin plugin and allow a default_header_type type to be able to be set with opentelemetry.

This even aligns with what OpenTelemtry states should be the default:

https://opentelemetry.io/docs/concepts/signals/traces/#context-propagation

Of note, even if this is allowed to be set, especially if set to w3c it will also need to ensure that 8byte trace id headers don't get to the backend. This could be accomplished a few ways:

  • Convert 8byte trace id's to 16byte trace id's
  • If 8byte found, Kong can convert it to a new 16byte trace id
  • Throw away the 8byte trace id and don't populate the trace id

Steps To Reproduce

  1. On Kong 3.1.1 set opentelemetry as your tracing plugin
  2. Make a call that has a b3, 8 byte trace id
  3. You'll find the backend doesn't have a w3c supported propagation

Anything else?

No response

@nbialostosky
Copy link
Contributor Author

FWIW, it does look like part of the issue reported was fixed with this: #10332

However, we still need the w3c headers to get set and passed along to the backend.

@hbagdi
Copy link
Member

hbagdi commented Mar 2, 2023

@nbialostosky Are you up for a contribution?

@nbialostosky
Copy link
Contributor Author

@hbagdi I took a shot at implementing this here: #10620

nbialostosky added a commit to nbialostosky/kong that referenced this issue Apr 13, 2023
At the moment the opentelemtry plugin sets the `default_header_type` for propigations:

https://github.com/Kong/kong/blob/master/kong/plugins/opentelemetry/handler.lua#L150

However, doesn't allow the modification of `header_type` in the `schema`, so it always defaults to `preserve`. However, based on the function it's calling:

https://github.com/Kong/kong/blob/master/kong/tracing/propagation.lua#L432

> -- If conf_header_type is set to `preserve`, found_header_type is used over default_header_type;

The above means the `default_header_type` is never used. We want the `default_header_type` to be used, and we want to ignore the incoming header type. This commit takes care of addressing this issue:

Kong#10246

I am not implmenting the ability to modify the `default_header_type` as opentelemetry notes it should default to `w3c` which it currently is:

https://opentelemetry.io/docs/concepts/signals/traces/#context-propagation
samugi added a commit that referenced this issue Apr 13, 2023
* feat(opentelemetry): Allow header_type to be set

At the moment the opentelemtry plugin sets the `default_header_type` for propigations:

https://github.com/Kong/kong/blob/master/kong/plugins/opentelemetry/handler.lua#L150

However, doesn't allow the modification of `header_type` in the `schema`, so it always defaults to `preserve`. However, based on the function it's calling:

https://github.com/Kong/kong/blob/master/kong/tracing/propagation.lua#L432

> -- If conf_header_type is set to `preserve`, found_header_type is used over default_header_type;

The above means the `default_header_type` is never used. We want the `default_header_type` to be used, and we want to ignore the incoming header type. This commit takes care of addressing this issue:

#10246

I am not implmenting the ability to modify the `default_header_type` as opentelemetry notes it should default to `w3c` which it currently is:

https://opentelemetry.io/docs/concepts/signals/traces/#context-propagation

* Update kong/plugins/opentelemetry/schema.lua

---------

Co-authored-by: Samuele Illuminati <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants