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

feat(opentelemetry): Allow header_type to be set #10620

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

nbialostosky
Copy link
Contributor

@nbialostosky nbialostosky commented Apr 5, 2023

Summary

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.

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

Checklist

Full changelog

  • Implement the ability to set header_type for the opentelemetry plugin

Issue reference

Fix #10246

@nbialostosky
Copy link
Contributor Author

Note: I couldn't get the tests to run locally with make test or with gojira:

gojira up
gojira run make dev
gojira run busted spec/03-plugins/37-opentelemetry

When I attempted the gojira command, it was returning a bazel error. I was hoping that CI would take care of running the test to confirm this functionality once I raised the PR.

Copy link
Member

@samugi samugi left a comment

Choose a reason for hiding this comment

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

looks great! Just some small observations:

spec/03-plugins/37-opentelemetry/03-propagation_spec.lua Outdated Show resolved Hide resolved
kong/plugins/opentelemetry/schema.lua Outdated Show resolved Hide resolved
@samugi
Copy link
Member

samugi commented Apr 7, 2023

I couldn't get the tests to run locally

It is possible that you'll have to run gojira run make dev-legacy, there may be some temporary issues with the Bazel build that we are working on.

There's an entry in the CHANGELOG note, it specifically says not to edit this

ah! Thanks for pointing that out, we will fix that, please do feel free to update the CHANGELOG

[?] Can submit documentation if needed

that would be awesome! ❤️ the docs for this plugin can be found on this page.

@nbialostosky
Copy link
Contributor Author

FWIW I still couldn't get this working locally with gojira run make dev-legacy, I was getting:

─$ gojira run busted spec/03-plugins/37-opentelemetry
✱✱✱✱✱
0 successes / 0 failures / 5 errors / 0 pending : 0.223766 seconds

Error → ./kong/tools/utils.lua @ 37
suite spec/03-plugins/37-opentelemetry/01-otlp_spec.lua
./kong/tools/utils.lua:37: attempt to index global 'ngx' (a nil value)

ah! Thanks for pointing that out, we will fix that, please do feel free to update the CHANGELOG

Thanks! Went ahead and added a note in the CHANGELOG with the recent commit.

that would be awesome!

Working on the docs now, will post a link here once that's done!

nbialostosky added a commit to nbialostosky/docs.konghq.com that referenced this pull request Apr 11, 2023
This pull request will add the ability to set the `header_type` for the
opentelemetry plugin:

Kong/kong#10620

To ensure the documentation remains in sync with the options available
for plugins, this adds the `header_type` key to the `config` section
showing its something that can be set.
@nbialostosky
Copy link
Contributor Author

Added documentation here: Kong/docs.konghq.com#5422

Wasn't sure what to point at, so pointed to main, since not sure when this PR will get approved/merged/released.

@samugi
Copy link
Member

samugi commented Apr 12, 2023

FWIW I still couldn't get this working locally with gojira run make dev-legacy, I was getting:

─$ gojira run busted spec/03-plugins/37-opentelemetry
✱✱✱✱✱
0 successes / 0 failures / 5 errors / 0 pending : 0.223766 seconds

Error → ./kong/tools/utils.lua @ 37
suite spec/03-plugins/37-opentelemetry/01-otlp_spec.lua
./kong/tools/utils.lua:37: attempt to index global 'ngx' (a nil value)

ah! Thanks for pointing that out, we will fix that, please do feel free to update the CHANGELOG

Thanks! Went ahead and added a note in the CHANGELOG with the recent commit.

that would be awesome!

Working on the docs now, will post a link here once that's done!

@nbialostosky alright that makes sense, please use gojira run bin/busted spec/03-plugins/37-opentelemetry instead.
This is just an FYI, I'm approving the CI runs now.

nbialostosky added a commit to nbialostosky/docs.konghq.com that referenced this pull request Apr 12, 2023
This pull request will add the ability to set the `header_type` for the
opentelemetry plugin:

Kong/kong#10620

To ensure the documentation remains in sync with the options available
for plugins, this adds the `header_type` key to the `config` section
showing its something that can be set.
nbialostosky added a commit to nbialostosky/docs.konghq.com that referenced this pull request Apr 12, 2023
This pull request will add the ability to set the `header_type` for the
opentelemetry plugin:

Kong/kong#10620

To ensure the documentation remains in sync with the options available
for plugins, this adds the `header_type` key to the `config` section
showing its something that can be set.
@nbialostosky
Copy link
Contributor Author

New PR link for the docs pointed at the proper release branch: Kong/docs.konghq.com#5431

mheap pushed a commit to nbialostosky/docs.konghq.com that referenced this pull request Apr 12, 2023
This pull request will add the ability to set the `header_type` for the
opentelemetry plugin:

Kong/kong#10620

To ensure the documentation remains in sync with the options available
for plugins, this adds the `header_type` key to the `config` section
showing its something that can be set.
@nbialostosky
Copy link
Contributor Author

This is just an FYI, I'm approving the CI runs now.

Thanks! To close the loop, the command you provided worked and I was able to successfully run the tests locally, they passed. I will share this information with my team so we are all able to test locally before raising upstream PR's.

CHANGELOG.md Outdated Show resolved Hide resolved
spec/03-plugins/37-opentelemetry/03-propagation_spec.lua Outdated Show resolved Hide resolved
spec/03-plugins/37-opentelemetry/03-propagation_spec.lua Outdated Show resolved Hide resolved
@samugi
Copy link
Member

samugi commented Apr 13, 2023

looks great @nbialostosky! This just needs a rebase to resolve a couple of conflicts and then it'll be ready to merge. Thanks!

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
@nbialostosky
Copy link
Contributor Author

@samugi Thanks! Went ahead and rebased and resolved the conflicts, lemme know if you need anything else from my end!

@samugi samugi merged commit 009ee44 into Kong:master Apr 13, 2023
lena-larionova added a commit to Kong/docs.konghq.com that referenced this pull request Apr 13, 2023
* Add header_type to OTel documentation

This pull request will add the ability to set the `header_type` for the
opentelemetry plugin:

Kong/kong#10620

To ensure the documentation remains in sync with the options available
for plugins, this adds the `header_type` key to the `config` section
showing its something that can be set.

* Update app/_hub/kong-inc/opentelemetry/_index.md

Co-authored-by: lena-larionova <[email protected]>

* Remove required: true to match change

The change ended up marking the schema to be required: false, so removing this line as it's no longer releveant.

---------

Co-authored-by: lena-larionova <[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 this pull request may close these issues.

Allow w3c to be set as the default propagation for opentelemetry plugin
3 participants