Migrate jaeger to otel API #7547
Conversation
tekton-robot
added
release-note-none
|
Hi @kmjayadeep. Thanks for your PR. I'm waiting for a tektoncd 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/test-infra repository. |
tekton-robot
added
needs-ok-to-test
|
/kind bug |
kmjayadeep
force-pushed
the
oltp-migrate
branch
from
acff709 to
f274592
Compare
tekton-robot
removed
the
needs-rebase
| return trace.NewNoopTracerProvider(), nil | ||
| return noop.NewTracerProvider(), nil | ||
| } | ||
| u, err := url.Parse(cfg.Endpoint) |
There was a problem hiding this comment.
Jaeger exporter expected a complete URL path like http://website.com:port/path, while otlp exporter accepts host and path as separate arguments. This URL parsing is added to ensure that user experience has not changed.
|
/ok-to-test |
tekton-robot
added
ok-to-test
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-go-coverage-df |
|
@vdemeester: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
The following is the coverage report on the affected files.
|
tekton-robot
added
the
needs-rebase
|
@kmjayadeep Could you please rebase this PR, so we merge it in time for v0.58? |
Otel SDK is upgraded from 1.19 to 1.21 Jaeger exporter is deprecated in otel 1.20, in favor of otel exporter Migrated jaeger to otel http exporter Signed-off-by: Jayadeep KM <[email protected]>
tekton-robot
removed
the
needs-rebase
|
@afrittoli The PR branch is rebased with main |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
| @@ -150,12 +153,32 @@ func createTracerProvider(service string, cfg *config.Tracing, user, pass string | |||
| if !cfg.Enabled { | |||
| return noop.NewTracerProvider(), nil | |||
| } | |||
| u, err := url.Parse(cfg.Endpoint) | |||
There was a problem hiding this comment.
@kmjayadeep question: why do we need the additional configuration and the config-tracing configmap when it seems like otel can be configured with env vars (https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp#pkg-overview)?
Should we just have one way to configure tracing? (the env vars seem to support more options too e.g. client certs etc. vs the config map which only supports basic auth)
There was a problem hiding this comment.
Hi @dibyom, we have used env vars initially. Then we decided to move to configmap to make it consistent with how other configurations are managed in the project. Also, env variables requires the user to restart the controller when the configuration needs to be changed.
Env variables are still supported. We can also add more parameters in the configmap as needed.
| if user != "" && pass != "" { | ||
| creds := fmt.Sprintf("%s:%s", user, pass) | ||
| enc := base64.StdEncoding.EncodeToString([]byte(creds)) | ||
| o := otlptracehttp.WithHeaders(map[string]string{ | ||
| "Authorization": fmt.Sprintf("Basic %s", enc), | ||
| }) | ||
| opts = append(opts, o) | ||
| } |
There was a problem hiding this comment.
Is it an option to have a user with empty password?
There was a problem hiding this comment.
What I found during testing with jaeger and nginx is that if the username or password is configured to be empty, the server just ignores the whole authorization header. So no need to pass the header in code.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/lgtm |
|
/test pull-tekton-pipeline-beta-integration-tests |
|
/test pull-tekton-pipeline-go-coverage-df |
|
@chitrangpatel: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
|
/retest |
|
Not sure why the test is failing. Seems like a glitch. It is unable to run the second Task I think: https://dashboard.dogfooding.tekton.dev/#/namespaces/tekton-ci/pipelineruns/run-go-coverage-hswz7?pipelineTask=split-full-repo-name&step=split-name I wonder if closing and reopening the PR will help her 🤔 ? |
|
@chitrangpatel I have raised an identical PR in #7748. Please feel free to close this one if that one gets through |
Changes
OpenTelemetry has deprecated the use of jaeger exporter directly and it has been removed from the library since 1.20 (changelog)
This PR migrates the code to make it compatible with the recommended oltptracehttp exporter instead. Also fixes issues with broken imports when upgrading to otel 1.21
Fixes #7464
/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes