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

Working OpenTelemetry sidecar (without base nginx image) #8735

Closed
wants to merge 7 commits into from

Conversation

Tobrek
Copy link
Contributor

@Tobrek Tobrek commented Jun 23, 2022

This pull request is part 2 of 2 of the split of PR #8622 (obsolete now). It contains all the changes from #8622 (and some more) beside the base nginx image adaptions, which are located in #8719 . For previous conversation see #8622 .

What this PR does / why we need it:

This is a working rudimentary support of OpenTelemetry sidecar. Currently just loading the OpenTelemetry module and the config at http block is supported. All other OpenTelemetry nginx directives (https://github.com/open-telemetry/opentelemetry-cpp-contrib/tree/main/instrumentation/nginx#nginx-directives) are not supported right now.

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)
  • Documentation only

Which issue/s this PR fixes

fixes #8437
#5883

How Has This Been Tested?

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    As best as i could and what i found. Maybe more must be updated.
  • I've read the CONTRIBUTION guide
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jun 23, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @Tobrek. 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 k8s-ci-robot added needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 23, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Tobrek
To complete the pull request process, please assign rikatz after the PR has been reviewed.
You can assign the PR to them by writing /assign @rikatz in a comment when ready.

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
Copy link
Contributor

@longwuyuan: The label(s) area/stabilisation cannot be applied, because the repository doesn't have them.

In response to this:

/area stabilisation

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.

@longwuyuan
Copy link
Contributor

/area stabilization

@k8s-ci-robot k8s-ci-robot added the area/stabilization Work for increasing stabilization of the ingress-nginx codebase label Jun 24, 2022
@longwuyuan
Copy link
Contributor

@Tobrek ,

(1) In this PR we will only do this ;

  • Compile OTEL
  • Add the OTEL-Collector so that we can talk to jaeger/zipkin (@esigo help needed on this)
  • Enable the OTEL-Module

(2) Urgently, we need a new PR for ;

  • RUN echo "/lib:/usr/lib:/usr/local/lib:/modules_mount/otel/lib" > /etc/ld-musl-x86_64.path

(3) The Go files will be taken care of later in yet another PR that we don't need to create now

Your compilation and module-enabling code has to be discussed

Thank you both so much. Will wait for next time we can sync and make progress (either on slack or maybe another zoom session - will try to keep it shorter)

@tao12345666333
Copy link
Member

/project Stabilization Project

@longwuyuan
Copy link
Contributor

@tao12345666333 @strongjz the PR is not visible on the project page. Is it just me or can you also not see it

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2022
@Tobrek
Copy link
Contributor Author

Tobrek commented Jun 24, 2022

(1) In this PR we will only do this ;
[...]
* Enable the OTEL-Module
[...]
(3) The Go files will be taken care of later in yet another PR that we don't need to create now

If i move all the go file changes to a separate PR, how should i enable the sidecar?
If i use the config to enable the sidecar, then i need (at least some of) the go files. Otherwise i have to enable the sidecar by default. Or the "go files" PR will be merged before this one.

@longwuyuan
Copy link
Contributor

hi @Tobrek ,

With the new nginx base-image and the ld-musl.path file in container image, the current status is as below screenshot

otel_module_loads_now

So in this PR we should limit ourself to automating the loading of the module. Then have another PR for steps beyond that. That is plan mentioned by @rikatz and also in a few days there will be some more engagement

@strongjz
Copy link
Member

strongjz commented Jul 2, 2022

/kind feature
/priority important-soon
/triage accepted

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 2, 2022
@k8s-ci-robot
Copy link
Contributor

@strongjz: The label(s) priority/ cannot be applied, because the repository doesn't have them.

In response to this:

/kind feature
/priority important-soon
/triage accepted

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 k8s-ci-robot removed needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 2, 2022
@stibi
Copy link
Contributor

stibi commented Aug 11, 2022

Hello 👋 I can see this is currently marked as "blocked" … what needs to be done to unblock it / finish it? I'd like to help, just if you can point me to the right direction.

@longwuyuan
Copy link
Contributor

@stibi Please ping me on slack. This is complicated, we have a plan on how to proceed. But there are highe-priority tasks in progress and there is lack of developers to do this in parallel.

@longwuyuan
Copy link
Contributor

longwuyuan commented Sep 5, 2022 via email

@k8s-ci-robot
Copy link
Contributor

@Tobrek: 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 k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2022
@Tobrek
Copy link
Contributor Author

Tobrek commented Sep 6, 2022

Close in advantage of #9016

@Tobrek Tobrek closed this Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs area/stabilization Work for increasing stabilization of the ingress-nginx codebase cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

An error occurred between opentelemetry modules
6 participants