kubernetes / ingress-nginx

Ingress NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.56k stars 8.27k forks source link

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

Open tsimonitoring opened 1 month ago

tsimonitoring commented 1 month ago

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: https://github.com/tsimonitoring/ingress-nginx/pull/9 release 1.11: https://github.com/tsimonitoring/ingress-nginx/pull/10 release 1.12: https://github.com/tsimonitoring/ingress-nginx/pull/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 https://github.com/kubernetes/ingress-nginx/issues/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 https://github.com/open-telemetry/opentelemetry-cpp-contrib/pull/443 or in detail the commit: https://github.com/open-telemetry/opentelemetry-cpp-contrib/commit/415f1824df55d8c35793c72af68742d7f999b3e2#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.

k8s-ci-robot commented 1 month ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
longwuyuan commented 1 month ago

/assign @esigo

matthias-haase commented 1 week ago

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

tsimonitoring commented 1 week ago

for short:

Here is my pull request intern in my repo according: release 1.10: https://github.com/tsimonitoring/ingress-nginx/pull/9 release 1.11: https://github.com/tsimonitoring/ingress-nginx/pull/10 release 1.12: https://github.com/tsimonitoring/ingress-nginx/pull/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 commented 1 week ago

Can you open a PR for the main project please?

Thanks

tsimonitoring commented 1 week ago

@rikatz see pull request: https://github.com/kubernetes/ingress-nginx/pull/12371

tsimonitoring commented 1 week ago

@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: https://github.com/kubernetes/ingress-nginx/pull/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 ?