ingress-nginx icon indicating copy to clipboard operation
ingress-nginx copied to clipboard

OpenTelemetry support

Open dmathieu opened this issue 4 years ago • 17 comments

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.

dmathieu avatar Sep 10 '21 14:09 dmathieu

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:

k8s-ci-robot avatar Sep 10 '21 14:09 k8s-ci-robot

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.

k8s-ci-robot avatar Sep 10 '21 14:09 k8s-ci-robot

wow! Thanks

tao12345666333 avatar Sep 10 '21 14:09 tao12345666333

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!

rikatz avatar Sep 10 '21 17:09 rikatz

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).

dmathieu avatar Sep 10 '21 20:09 dmathieu

Amazing news. This should simplify some things A LOT! :)

rikatz avatar Sep 10 '21 21:09 rikatz

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.

duhow avatar Oct 31 '21 14:10 duhow

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.

dmathieu avatar Nov 02 '21 08:11 dmathieu

Is it still blocked due to the base image?

indrekj avatar Nov 23 '21 12:11 indrekj

Yes. This hasn't moved forward yet.

dmathieu avatar Dec 01 '21 13:12 dmathieu

@dmathieu would you be open to looking and commenting on https://github.com/kubernetes/ingress-nginx/issues/8437

longwuyuan avatar Apr 06 '22 06:04 longwuyuan

https://github.com/open-telemetry/opentelemetry-cpp-contrib

longwuyuan avatar Apr 08 '22 05:04 longwuyuan

/kind feature /ok-to-test /priority backlog /triage accepted

strongjz avatar Jun 15 '22 20:06 strongjz

@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.

k8s-ci-robot avatar Jun 15 '22 20:06 k8s-ci-robot

CLA Missing ID CLA Not Signed

  • :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.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Jul 22 '22 09:07 k8s-ci-robot

@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.

k8s-ci-robot avatar Aug 08 '22 23:08 k8s-ci-robot

@dmathieu, are you looking for help on this PR?

bixu avatar Sep 27 '22 08:09 bixu

@bixu https://github.com/kubernetes/ingress-nginx/pulls/esigo

longwuyuan avatar Sep 27 '22 08:09 longwuyuan

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 avatar Sep 27 '22 08:09 dmathieu

@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 avatar Jan 08 '23 22:01 rikatz

@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.

k8s-ci-robot avatar Jan 08 '23 22:01 k8s-ci-robot

@rikatz do you have a link to the work that this was closed in favor of?

mjallday avatar Jan 09 '23 01:01 mjallday

@mjallday, I think you are looking for https://github.com/kubernetes/ingress-nginx/pulls/esigo

bixu avatar Jan 09 '23 16:01 bixu