ingress-nginx
ingress-nginx copied to clipboard
OpenTelemetry support
What this PR does / why we need it:
This PR adds support for OpenTelemetry (using the official plugin), to send request traces to any otel collector.
This is still a draft, but I'm opening a PR anyway to start getting potential feedback. The missing things are testing, as well as https://github.com/open-telemetry/opentelemetry-cpp-contrib/pull/52 to avoid using my own fork.
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
Which issue/s this PR fixes
This has been proposed in #5883
How Has This Been Tested?
This is being tested through the E2E tests. Before moving this out of draft, I will be running it in a test cluster as well.
Checklist:
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I've read the CONTRIBUTION guide
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.
Welcome @dmathieu!
It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes/ingress-nginx has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @dmathieu. Thanks for your PR.
I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
wow! Thanks
A question, from someone that does not know how OT works properly:
If we add OT support, can we remove all the jaeger and opentracing and have just one additional library?
Thanks for the PR!
Yes. Opentracing is deprecated anyway. As for jaeger, they support OTLP today and their official position is that in the future, they will only support OTLP (the OpenTelemetry GRPC format).
Amazing news. This should simplify some things A LOT! :)
While this is still pending to merge, how can I do to manually deploy this in my Kubernetes cluster? I wanted to use OpenTracing but latest nginx-controller version gives an error, and also since it's going to be deprecated by this OpenTelemetry, I'd like to setup it.
You can't. This is still WIP. We should have the CPP plugin installed in the base image, but there are what seems to be timeouts preventing the image from being built/pushed.
Is it still blocked due to the base image?
Yes. This hasn't moved forward yet.
@dmathieu would you be open to looking and commenting on https://github.com/kubernetes/ingress-nginx/issues/8437
https://github.com/open-telemetry/opentelemetry-cpp-contrib
/kind feature /ok-to-test /priority backlog /triage accepted
@dmathieu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-ingress-nginx-test | 5611699a18eb8a7009439966e0209c3b9b7aeecd | link | true | /test pull-ingress-nginx-test |
| pull-ingress-nginx-codegen | 5611699a18eb8a7009439966e0209c3b9b7aeecd | link | true | /test pull-ingress-nginx-codegen |
| pull-ingress-nginx-boilerplate | 5611699a18eb8a7009439966e0209c3b9b7aeecd | link | true | /test pull-ingress-nginx-boilerplate |
| pull-ingress-nginx-gofmt | 5611699a18eb8a7009439966e0209c3b9b7aeecd | link | true | /test pull-ingress-nginx-gofmt |
| pull-ingress-nginx-golint | 5611699a18eb8a7009439966e0209c3b9b7aeecd | link | true | /test pull-ingress-nginx-golint |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.
- :white_check_mark: login: dmathieu / name: Damien Mathieu (4d3031563a6c2741f52a5bf685b72dec09935008, bdf6b08f7b56d2908cd0b38f4cabd0d7d800f40d, 7ad90a3ee0797a6ba9356065d89dddccfaa0c90f, b29de2569cbea8583b3e9e3e5d0c609c61da409e, 527ab267e87d5e6413c34b6094869fe112d6f706, c3e9fcb08536447d0d4370d614e4125aca61bccf, 355cb4d8e93678771db7b6c785176a011ff86b02, adf013c284323cc09975afd97eedcc86ad83cdae, 7af047425628db435b1904e6046c37133d501f81, 41b5e3e75f4b2c04b5d0710dc4bc015e34e9e3d2, 8bb032653771fc30b13c94014208bbaa3fda3ad0, 5fe07254e1fcccb0d3ddc0ea917dce03325fefb0, 403a6b2fbbbb54b083ca160b0ec90345190e3c8b)
- :x: The commit (64b69350b7b7250f2c36556d6bb94c1fb3384648, da25d4d5aaaf92fdc934e00ac1e72cf1eae5daf6, ff6066aa7f59d6b9589f3b9a46e161d5774f45e5, 01327b9fb559bda686264542d9ced2dc70d10a2c). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: dmathieu
Once this PR has been reviewed and has the lgtm label, please assign elvinefendi for approval by writing /assign @elvinefendi in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@dmathieu: PR needs rebase.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@dmathieu, are you looking for help on this PR?
@bixu https://github.com/kubernetes/ingress-nginx/pulls/esigo
I have fallen a bit behind on what was happening with this work. And it seems other folks are more up to date than me. If somebody wants to take over, I would be happy with that.
@dmathieu I'm closing this in favor of some work @esigo has been doing on OTEL, but if I'm wrong please feel free to reopen and reach me on Slack!!
Thank you for your effort on this one! /close
@rikatz: Closed this PR.
In response to this:
@dmathieu I'm closing this in favor of some work @esigo has been doing on OTEL, but if I'm wrong please feel free to reopen and reach me on Slack!!
Thank you for your effort on this one! /close
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
@rikatz do you have a link to the work that this was closed in favor of?
@mjallday, I think you are looking for https://github.com/kubernetes/ingress-nginx/pulls/esigo