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

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

Open
tsimonitoring opened this issue Oct 18, 2024 · 7 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@tsimonitoring
Copy link

tsimonitoring commented Oct 18, 2024

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

That's why ask:

Can you please change the follow files with lines:
.
from my side i changes some lines in main git ingress-nginx path

diff --git a/images/nginx/rootfs/Dockerfile b/images/nginx/rootfs/Dockerfile

-FROM alpine:3.20 as builder
+FROM alpine:3.20 AS builder
...
...apk upgrade...
+ abseil-cpp-crc-cpu-detect \

diff --git a/images/nginx/rootfs/build.sh b/images/nginx/rootfs/build.sh

-export OPENTELEMETRY_CPP_VERSION="v1.11.0"
+export OPENTELEMETRY_CPP_VERSION="v1.17.0"
-export OPENTELEMETRY_PROTO_VERSION="v1.1.0"
+export OPENTELEMETRY_PROTO_VERSION="v1.3.2"
-export OPENTELEMETRY_CONTRIB_COMMIT=e11348bb400d5472bf1da5d6128bead66fa111ff
+export OPENTELEMETRY_CONTRIB_COMMIT=f6d29426ee9b4d6b476c09ca3cb9bed3cf23906f

This will solve the problem.

Tested in test env on my side successful.

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

@tsimonitoring tsimonitoring added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 18, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 18, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@longwuyuan
Copy link
Contributor

/assign @esigo

@matthias-haase
Copy link

is there any possibility get a "triage accepted" according this request of change some code lines ?

@tsimonitoring
Copy link
Author

for short:

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

on main you can do same i did per script or bash command:

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
#
# AS builder
perl -pi -e "s/as builder/AS builder/g;" images/nginx/rootfs/Dockerfile
#
# abseil-cpp-crc-cpu-detect
perl -pi -e "s/(libprotobuf.*)/\1\n  abseil-cpp-crc-cpu-detect \\\/g;" images/nginx/rootfs/Dockerfile

@rikatz
Copy link
Contributor

rikatz commented Nov 14, 2024

Can you open a PR for the main project please?

Thanks

matthias-haase pushed a commit to matthias-haase/ingress-nginx that referenced this issue Nov 15, 2024
…::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 .
@tsimonitoring
Copy link
Author

tsimonitoring commented Nov 15, 2024

@rikatz see pull request: #12371

@tsimonitoring
Copy link
Author

tsimonitoring commented Nov 15, 2024

@rikatz @esigo
With azure kubernetes version 1.31 there is a needed pressure using newsest nginx.
Problem: newest nginx uses opentelemetry instead opentracing, but trace status error is not shipping in a correct way.
Due to need use correct monitoring with tracing there's a stop accoring go to newer versions with nginx.
That's why i created the pull request: #12371
How can someone push and go to a faster with minimal time delay integrate such a change of 3 lines in build.sh and 1 in Dockerfile in images/nginx/rootfs/ ?

You ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

Successfully merging a pull request may close this issue.

6 participants