Add configmap for tracing config #6897
Conversation
tekton-robot
added
the
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
kmjayadeep
changed the title
|
/kind misc |
|
The following is the coverage report on the affected files.
|
tekton-robot
added
ok-to-test
|
The following is the coverage report on the affected files.
|
kmjayadeep
force-pushed
the
tracing-configmap
branch
2 times, most recently
from
6ff5163
to
e42358e
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
kmjayadeep
force-pushed
the
tracing-configmap
branch
from
e42358e
to
e26a259
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
kmjayadeep
force-pushed
the
tracing-configmap
branch
from
e26a259
to
977a4be
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
kmjayadeep
force-pushed
the
tracing-configmap
branch
from
977a4be
to
bdc1101
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Signed-off-by: Jayadeep KM <[email protected]>
kmjayadeep
force-pushed
the
tracing-configmap
branch
from
bdc1101
to
86c4401
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
thanks @kmjayadeep! would you mind adding some more context to your commit message/PR description on the reason for this change? Also, this likely needs a release note.
| # | ||
| # API endpoint to send the traces to | ||
| # (optional): The default value is given below | ||
| endpoint: "http://jaeger-collector.jaeger.svc.cluster.local:14268/api/traces" |
There was a problem hiding this comment.
would it make sense to either:
- assume that if someone has configured an endpoint, that they want tracing enabled, i.e. remove the "enabled" setting
- make this configuration real configuration instead of an example configuration, and set this to be the actual default endpoint and set enabled to "false" by default
?
There was a problem hiding this comment.
assume that if someone has configured an endpoint, that they want tracing enabled, i.e. remove the "enabled" setting
We will add more config options later. So it makes sense to make the enable flag explicit rather than based on the endpoint, which will be confusing to the users.
make this configuration real configuration instead of an example configuration, and set this to be the actual default endpoint and set enabled to "false" by default
We follow this pattern of giving only an example for other configmaps like config-events and config-observability. It is better to keep it consistent with those.
| @@ -54,7 +59,9 @@ func NewController(opts *pipeline.Options, clock clock.PassiveClock, tracerProvi | |||
| pipelineRunInformer := pipelineruninformer.Get(ctx) | |||
| resolutionInformer := resolutioninformer.Get(ctx) | |||
| verificationpolicyInformer := verificationpolicyinformer.Get(ctx) | |||
| configStore := config.NewStore(logger.Named("config-store"), pipelinerunmetrics.MetricsOnStore(logger)) | |||
| tracerProvider := tracing.New(TracerProviderName) | |||
| //nolint:contextcheck // OnStore methods does not support context as a parameter | |||
There was a problem hiding this comment.
rather than disabling the linter, would it make sense for OnStore to accept a ctx?
There was a problem hiding this comment.
Nope. It only makes sense if the OnStore method is directly consuming the context object.
But in this case, the OnStore method returns a callback function that is called every time the configmap changes. Ideally the callback method should be accepting the context object which is passed by the caller. But this logic is implemented inside knative library and we cannot change it.
That's why i had to create a context object inside the callback method. The linter is reporting a false positive error in this case. Of course we can pass the context and use it inside the callback, but it doesn't much sense to do it that way.
|
|
||
| * `OTEL_EXPORTER_JAEGER_ENDPOINT` is the HTTP endpoint for sending spans directly to a collector. | ||
| * `OTEL_EXPORTER_JAEGER_USER` is the username to be sent as authentication to the collector endpoint. |
There was a problem hiding this comment.
I'm curious why some of the configuration is moving to a configmap but not all of it. Is it because the password can be read from a secret when used in an envvar but not when used in a configmap? maybe the configmap could accept the name of a secret where the password is stored?
There was a problem hiding this comment.
Yes that's the idea. Configmap will have a secret name where creds are stored. It requires a separate logic for reconciling the secret. I wanted to keep the scope of the PR small for easy review process. I already have a WIP follow-up PR for this change in my fork here if you are interested to take a look.
| // tp.Tracer should return a nooptracer initially | ||
| // recording is always false for spans created by nooptracer | ||
| if span.IsRecording() { | ||
| t.Fatalf("Span is recording before configuration") |
There was a problem hiding this comment.
super nits: context.background makes more sense than context.TODO IMO, since there's nothing todo, and I think t.Errorf makes more sense than Fatalf, since it's asserting test outputs rather than failing some setup steps
There was a problem hiding this comment.
Other tests are likely to fail if one of these fails, so it doesn't make much difference in reality.
But you are right that it is a better practice to use t.Errorf instead. I will make these nit changes along with any other feedbacks as part of the PR. Otherwise I will just add it to my next PR if that's okay.
tekton-robot
added
release-note
I have updated the description and release notes. I think it is clear enough. Feel free to update it directly as required. |
There was a problem hiding this comment.
Thank you @kmjayadeep!
/lgtm
@lbernick are you happy with the replies with your comments? If so can we merge this now?
|
No problem with merging |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick 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
Changes
This PR moves the configuration of tracing from environment variables into a configmap named
config-tracing. The available configuration options are provided as an example in the configmap itself.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