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.

Sign up for GitHub

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

Jump to bottom

Fix Opentelemetry version and compilation #12370

Closed
wants to merge 1 commit into from
Closed

Fix Opentelemetry version and compilation #12370

wants to merge 1 commit into from
+4 −3

Conversation

matthias-haase
Copy link

@matthias-haase matthias-haase commented Nov 15, 2024 •
edited by rikatz
Loading

What this PR does / why we need it:

Bump Opentelemetry version to fix span sending, and fix Opentelemetry compilation

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • CVE Report (Scanner found CVE and adding report)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Which issue/s this PR fixes

fixes #12210

How Has This Been Tested?

Tested on Azure Kubernetes with datadog pods sending traces and check on datadog.com Dashboard.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have added unit and/or e2e tests to cover my changes.
  • All new and existing tests passed.

@tsimonitoring
Please correct controller and the ship correct "span->SetStatus(trace…
69a416b 
…::StatusCode...." and not "Ok" as default with all http_code. kubernetes#12210

OPENTELEMETRY_CPP_VERSION="v1.17.0"
perl -pi -e "s/(OPENTELEMETRY_CPP_VERSION=)(.*)/\1\"$OPENTELEMETRY_CPP_VERSION\"/g;" images/nginx/rootfs/build.sh
OPENTELEMETRY_PROTO_VERSION="v1.3.2"
perl -pi -e "s/(OPENTELEMETRY_PROTO_VERSION=)(.*)/\1\"$OPENTELEMETRY_PROTO_VERSION\"/g;" images/nginx/rootfs/build.sh
OPENTELEMETRY_CONTRIB_COMMIT=f6d29426ee9b4d6b476c09ca3cb9bed3cf23906f
perl -pi -e "s/(OPENTELEMETRY_CONTRIB_COMMIT=)(.*)/\1\"$OPENTELEMETRY_CONTRIB_COMMIT\"/g;" images/nginx/rootfs/build.sh
perl -pi -e "s/(libprotobuf.*)/\1\n  abseil-cpp-crc-cpu-detect \\\/g;" images/nginx/rootfs/Dockerfile

Ingress-NGINX 1.10.0 has dropped support for OpenTracing and Zipkin, favoring OpenTelemetry instead.

The OpenTelemetry module used by Ingress-NGINX is based on a old commit, and has received updates since then.

The correct value is not set according "span->SetStatus(trace::StatusCode::kError);".

Per default it's not correct set with "span->SetStatus(trace::StatusCode::kOk);" if there a trace with error (>=http_code 500).

Here is my pull request intern in my repo
according:
release 1.10: tsimonitoring/ingress-nginx#9
release 1.11: tsimonitoring/ingress-nginx#10
release 1.12: tsimonitoring/ingress-nginx#11

(in Datadog it's metric trace.nginx.server.errors.)

The changes according Ingress-NGINX 1.11.2 with my branch solved the problem according trace error status: https://github.com/tsimonitoring/ingress-nginx/tree/release-1.11.3-patch-opentelemetry-cpp-and-contrib-and-proto

As example tested on my side in Datadog.

There are correct OPENTELEMETRY_CPP_VERSION, OPENTELEMETRY_PROTO_VERSION, OPENTELEMETRY_CONTRIB_COMMIT in build.sh incl. apk upgrade abseil-cpp-crc-cpu-detect (add) in Dockerfile NGINX.

Before (https://i.imgur.com/LpvotMx.png) there was no shipped metric according error_status per OpenTelemetry Module.

After (https://i.imgur.com/xvz6b05.png) you can see the shipped error metric also in trace view or see diag example (https://i.imgur.com/xEEY2Ep.png).

Please see https://github.com/tsimonitoring/ingress-nginx/tree/release-1.11.3-patch-opentelemetry-cpp-and-contrib-and-proto

Is there currently another issue associated with this?
So far as i know there is no another issue associated with this (in this repo) according the "span->SetStatus(trace::StatusCode...." .

Hint: There's a solved issue kubernetes#11496, which solved shipping the http error code.

This issue i want to solve is: "Please correct controller and the ship correct "span->SetStatus(trace::StatusCode....".

OpenTelemetry module was updated with open-telemetry/opentelemetry-cpp-contrib#443
or in detail the commit:
open-telemetry/opentelemetry-cpp-contrib@415f182#diff-ac2154f3c67fc196193c979a092240e417392a11387cb1e2ba181828238cc8ffR551 .
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Nov 15, 2024 •
edited
Loading

CLA Not Signed

  • ❌ login: @tsimonitoring / The commit (69a416b). This user is authorized, but they must confirm their affiliation with their company. Start the authorization process by clicking here, click "Corporate", select the appropriate company from the list, then confirm your affiliation on the page that appears. For further assistance with EasyCLA, please submit a support request ticket.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 15, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @matthias-haase!

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

Hi @matthias-haase. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/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-priority size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 15, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: matthias-haase
Once this PR has been reviewed and has the lgtm label, please assign cpanato for approval. 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

@netlify Netlify
Copy link

netlify bot commented Nov 15, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name Link
🔨 Latest commit 69a416b
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/6737318e0c06a00008d3e31a

@tsimonitoring tsimonitoring mentioned this pull request Nov 15, 2024
Open
@rikatz rikatz changed the title Please correct controller and the ship correct "span->SetStatus(trace… Fix Opentelemetry version and compilation Nov 15, 2024
rikatz
rikatz reviewed Nov 15, 2024
View reviewed changes
images/nginx/rootfs/build.sh
@@ -512,7 +512,7 @@ make
make modules
make install

export OPENTELEMETRY_CONTRIB_COMMIT=e11348bb400d5472bf1da5d6128bead66fa111ff
export OPENTELEMETRY_CONTRIB_COMMIT="f6d29426ee9b4d6b476c09ca3cb9bed3cf23906f"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use latest from main here: "8933841f0a7f8737f61404cf0a64acf6b079c8a5"

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer using tagged releases here over just using latest main.

@rikatz
Copy link
Contributor

rikatz commented Nov 15, 2024

@matthias-haase do you mind signing the CLA?

Also please follow the PR template here, refer to the original issue with all the descriptions but on the PR just mention you are fixing Opentelemtry compilation and Version.

Thanks!

/ok-to-test
/triage accepted
/priority important-soon
/kind bug
/cc @esigo
/assign @esigo

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Nov 15, 2024
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/bug Categorizes issue or PR as related to a bug. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 15, 2024
@rikatz
Copy link
Contributor

rikatz commented Nov 15, 2024

(I will actually edit the PR and fix its description, please just be sure to follow it next time)

@Gacko
Copy link
Member

Gacko commented Nov 15, 2024

/close

Duplicate of #12371.

@k8s-ci-robot
Copy link
Contributor

@Gacko: Closed this PR.

In response to this:

/close

Duplicate of #12371.

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esigo esigo esigo left review comments

@rikatz rikatz rikatz left review comments

@Gacko Gacko Gacko left review comments

@tao12345666333 tao12345666333 Awaiting requested review from tao12345666333

Assignees

@esigo esigo

Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
6 participants
@matthias-haase @k8s-ci-robot @rikatz @Gacko @esigo @tsimonitoring