NGINX: Bump OpenTelemetry. #12371
NGINX: Bump OpenTelemetry. #12371
Conversation
…rfile-apk-abseil-cpp-crc-cpu-detect 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). (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).
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: matthias-haase 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 |
k8s-ci-robot
added
cncf-cla: yes
|
This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The 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. |
|
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 Once the patch is verified, the new status will be reflected by the 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
added
needs-kind
✅ Deploy Preview for kubernetes-ingress-nginx canceled.
|
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/kind feature |
k8s-ci-robot
added
kind/feature
|
Why is abseil-cpp-crc-cpu-detect needed? |
|
@strongjz abseil-cpp-crc-cpu-detect in Dockerfile needed by shared object. otel_ngx_module.so -> libopentelemetry_exporter_otlp_grpc.so -> libabsl_crc_cpu_detect.so.2308.0.0 Error message clearify this - without libabsl_crc_cpu_detect.so.2308.0.0 you get the errormessage: here are the repos, which work "together" OPENTELEMETRY_PROTO_VERSION -> https://github.com/open-telemetry/opentelemetry-proto/releases OPENTELEMETRY_CONTRIB_VERSION -> https://github.com/open-telemetry/opentelemetry-cpp-contrib/releases Answer: OPENTELEMETRY_CONTRIB_COMMIT creates libopentelemetry_exporter_otlp_grpc.so, which needs abseil-cpp-crc-cpu-detect. Proof: The other repos do not have a entry like "libopentelemetry_exporter_otlp_grpc". @strongjz in hope this helps push a faster commit, because: 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 according go to newer versions with nginx. An this stopps using next kubernetes version 1.31, which makes the pressure from azure. 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/ ? |
|
Also we are currently in the process of releasing v1.12. This change won't make it in there and will earliest be included in v1.13. Additionally we are currently working on bumping the NGINX to OpenResty v1.27, so let's just postpone this here until we bumped NGINX itself and try to integrate it based on that. |
Hello, |
Is there a way also update release v1.10 and v1.11 and 1.12, because there are a lot of deployments using a "buggy" opentelemetry module with not correct shipped trace error status? Background: This would helps test with current deploments in current kubernetes deployments and help go forward to new versions without risk in e2e-tests, incl. possibility if there's a "go back", you cab use the patch with older versions. Hint: 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/ incl. the v1.10 and v1.11 and 1.12 ? THX for hint according older and current versions. You are the best! |
|
Please do not file separate PRs on different branches. Back-porting changes is up to the maintainers of this project. Also I'd like to note that at least I'm feeling a little pushed by you. I understand this might be urgent to you (or your employer), but still we are all doing this in our free time and are responsible for maintaining changes brought to us by contributors. So I'd like to ask you for patience while we are reviewing your proposal thoroughly. |
|
One additional note, also to other maintainers: The compilation of the NGINX base image is still broken at the moment. I'd highly appreciate not merging any changes to it as long as it hasn't been fixed. This just makes it more complicated. |
Yes of course. You're right. Thank you review the pull request. |
matthias-haase
changed the title
Fix renamed. Ok ? |
|
Yes, sure. As already mentioned we cannot merge this for now as we still have some other changes on the NGINX base image in the pipeline. Thank you for your contribution so far! |
matthias-haase
changed the title
Gacko
changed the title
|
I updated the title as this is what you're actually doing. The exact intention can be found in the details, but the effect to users is literally just the bump. |
Thank you. Do i have to do now everything ? I ask, because i see a line: |
|
We will take care from here. As stated earlier we first need to fix the NGINX compilation anyway. |
is there a link to a log? can i help according the problem? |
|
There's a log, sure, but it's basically just an architecture / cross-compiling issue we need to figure out. Already had this in the past. Anyway, just for clarification and to not disappoint any expectations: Our current roadmap and priorities look as follows:
So there is currently no hurry to get this PR merged as we have other priorities for now and probably won't have in v1.12. |
|
Hello @matthias-haase! Well, it seeeeeeems like we could actually need this change in v1.12.0 already. 😅 I was just working on the before mentioned fix of the NGINX base image. Bumping the LuaJIT dependency (not related to this PR) made it work... until the whole process tries to compile OpenTelemetry stuff: I will now try to compile the NGINX base image with your changes included. If this works, I'll try to get both our changes in one PR (probably this one) without changing the authors, so it's getting to Important to note: We cannot have these changes merged to Does this sound good to you? Regards |
@Gacko Yes - sounds very good to me. THX a lot! My changes will not help if you use the newest versions of OPENTELEMETRY_CPP_VERSION="v1.18.0" or/and OPENTELEMETRY_PROTO_VERSION="v1.4.0". Do not know why the error occurs with new versions. I will also check, what's wrong. Do not know if i will find out the reason. My findings until now: conclusion i assume opentelemetry-cpp.git has the proble set correct PROTO_PATH + GENERATED_PROTOBUF_PATH OPENTELEMETRY_PROTO_VERSION="v1.3.2" changed to v1.4.0 and there's a bug. |
i did:
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).
(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).
What this PR does / why we need it:
Types of changes
Which issue/s this PR fixes
fixes # The correct value is not set according "span->SetStatus(trace::StatusCode::kError);".
How Has This Been Tested?
in azure kubernetes with test metric in datadog
Checklist: