OpenTelemetry support #7621

dmathieu · 2021-09-10T14:14:43Z

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 open-telemetry/opentelemetry-cpp-contrib#52 to avoid using my own fork.

Types of changes

Bug fix (non-breaking change which fixes an issue)
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.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2021-09-10T14:14:51Z

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

k8s-ci-robot · 2021-09-10T14:14:51Z

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.

tao12345666333 · 2021-09-10T14:16:03Z

wow! Thanks

rikatz · 2021-09-10T17:46:35Z

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!

dmathieu · 2021-09-10T20:57:23Z

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

rikatz · 2021-09-10T21:48:42Z

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

duhow · 2021-10-31T14:17:15Z

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.

dmathieu · 2021-11-02T08:13:48Z

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.

indrekj · 2021-11-23T12:51:50Z

Is it still blocked due to the base image?

dmathieu · 2021-12-01T13:28:36Z

Yes. This hasn't moved forward yet.

internal/ingress/controller/nginx_test.go

rootfs/etc/nginx/template/nginx.tmpl

internal/ingress/annotations/opentelemetry/main.go

rcjsuen · 2022-01-27T11:19:53Z

internal/ingress/controller/template/template.go

+	buf.WriteString("opentelemetry_config /etc/nginx/opentelemetry.toml;\r\n")
+
+	if cfg.OpenTelemetryOperationName != "" {
+		buf.WriteString(fmt.Sprintf("opentelemetry_operation_name \"%s\";\n", cfg.OpenTelemetryOperationName))


The first line uses \r\n but the second line uses \n. Should one be changed to match the other?

Fixed, though the PR is still a draft and I'm trying to avoid a big bang PR. I am therefore opening several smaller ones, and this one probably shouldn't be reviewed so carefully (yet).

I am therefore opening several smaller ones, and this one probably shouldn't be reviewed so carefully (yet).

That's a great idea!

Co-authored-by: Marshall Jones <[email protected]>

Co-authored-by: Remy Suen <[email protected]>

linux-foundation-easycla · 2022-07-22T09:43:28Z

✅ login: dmathieu / name: Damien Mathieu (4d30315, bdf6b08, 7ad90a3, b29de25, 527ab26, c3e9fcb, 355cb4d, adf013c, 7af0474, 41b5e3e, 8bb0326, 5fe0725, 403a6b2)
❌ The commit (64b6935, da25d4d, ff6066a, 01327b9). 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.

k8s-ci-robot · 2022-07-22T09:43:41Z

[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:

OWNERS

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 · 2022-08-08T23:34:09Z

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

bixu · 2022-09-27T08:21:11Z

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

longwuyuan · 2022-09-27T08:29:51Z

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

dmathieu · 2022-09-27T08:34:39Z

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.

rikatz · 2023-01-08T22:31:11Z

@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

k8s-ci-robot · 2023-01-08T22:31:16Z

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

mjallday · 2023-01-09T01:16:56Z

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

bixu · 2023-01-09T16:19:05Z

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

k8s-ci-robot added needs-priority size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 10, 2021

k8s-ci-robot requested review from bowei and tao12345666333 September 10, 2021 14:14

dmathieu mentioned this pull request Sep 20, 2021

add OpenTelemetry to nginx base image #7669

Merged

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 11, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021

dmathieu mentioned this pull request Dec 15, 2021

Support for OpenTelemetry in addition to OpenTracing? #5883

Closed

mjallday reviewed Dec 24, 2021

View reviewed changes

internal/ingress/controller/nginx_test.go Outdated Show resolved Hide resolved

internal/ingress/controller/nginx_test.go Outdated Show resolved Hide resolved

rcjsuen reviewed Dec 24, 2021

View reviewed changes

rootfs/etc/nginx/template/nginx.tmpl Outdated Show resolved Hide resolved

rcjsuen mentioned this pull request Jan 21, 2022

First sidecar module: OpenTelemetry #8013

Merged

9 tasks

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2022

rcjsuen reviewed Jan 27, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2022

Damien Mathieu and others added 14 commits July 22, 2022 11:36

Update internal/ingress/controller/nginx_test.go

64b6935

Co-authored-by: Marshall Jones <[email protected]>

Update internal/ingress/controller/nginx_test.go

da25d4d

Co-authored-by: Marshall Jones <[email protected]>

Update rootfs/etc/nginx/template/nginx.tmpl

ff6066a

Co-authored-by: Remy Suen <[email protected]>

setup opentelemetry trust incoming spans

b29de25

only enable OpenTelemetry if the module has been loaded

527ab26

fix typo

c3e9fcb

Update internal/ingress/annotations/opentelemetry/main.go

01327b9

Co-authored-by: Remy Suen <[email protected]>

add Makefile command to build all modules

355cb4d

build the modules in e2e tests

adf013c

setup WithDeploymentModule

7af0474

add opentelemetry module to e2e tests

41b5e3e

use \n only

8bb0326

build image modules in CI

5fe0725

always use the base image's entrypoint

403a6b2

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 22, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2022

k8s-ci-robot closed this Jan 8, 2023

dmathieu deleted the otel branch January 9, 2023 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenTelemetry support #7621

OpenTelemetry support #7621

dmathieu commented Sep 10, 2021

k8s-ci-robot commented Sep 10, 2021

k8s-ci-robot commented Sep 10, 2021

tao12345666333 commented Sep 10, 2021

rikatz commented Sep 10, 2021

dmathieu commented Sep 10, 2021

rikatz commented Sep 10, 2021

duhow commented Oct 31, 2021

dmathieu commented Nov 2, 2021

indrekj commented Nov 23, 2021

dmathieu commented Dec 1, 2021

rcjsuen Jan 27, 2022

dmathieu Feb 7, 2022

rcjsuen Feb 7, 2022

linux-foundation-easycla bot commented Jul 22, 2022

k8s-ci-robot commented Jul 22, 2022

k8s-ci-robot commented Aug 8, 2022

bixu commented Sep 27, 2022

longwuyuan commented Sep 27, 2022

dmathieu commented Sep 27, 2022

rikatz commented Jan 8, 2023

k8s-ci-robot commented Jan 8, 2023

mjallday commented Jan 9, 2023

bixu commented Jan 9, 2023

OpenTelemetry support #7621

OpenTelemetry support #7621

Conversation

dmathieu commented Sep 10, 2021

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Sep 10, 2021

k8s-ci-robot commented Sep 10, 2021

tao12345666333 commented Sep 10, 2021

rikatz commented Sep 10, 2021

dmathieu commented Sep 10, 2021

rikatz commented Sep 10, 2021

duhow commented Oct 31, 2021

dmathieu commented Nov 2, 2021

indrekj commented Nov 23, 2021

dmathieu commented Dec 1, 2021

rcjsuen Jan 27, 2022

Choose a reason for hiding this comment

dmathieu Feb 7, 2022

Choose a reason for hiding this comment

rcjsuen Feb 7, 2022

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jul 22, 2022

k8s-ci-robot commented Jul 22, 2022

k8s-ci-robot commented Aug 8, 2022

bixu commented Sep 27, 2022

longwuyuan commented Sep 27, 2022

dmathieu commented Sep 27, 2022

rikatz commented Jan 8, 2023

k8s-ci-robot commented Jan 8, 2023

mjallday commented Jan 9, 2023

bixu commented Jan 9, 2023