Replace returned Trace with Context argument in internal Applier interface #20307
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AwesomePatrol 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 |
|
Hi @AwesomePatrol. Thanks for your PR. I'm waiting for a etcd-io 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. |
AwesomePatrol
force-pushed
the
migration-to-otel-tracing-first-step
branch
from
d26a817 to
fb00ff4
Compare
AwesomePatrol
changed the title
AwesomePatrol
force-pushed
the
migration-to-otel-tracing-first-step
branch
2 times, most recently
from
ecb7674 to
1ec3f46
Compare
| case r.Range != nil: | ||
| op = "Range" | ||
| ar.Resp, ar.Trace, ar.Err = a.applyV3.Range(r.Range) | ||
| ctx := context.Background() |
There was a problem hiding this comment.
Do you need to have separate context constructor in each switch ? Moving it outside of switch should not be a problem and will be more readable.
There was a problem hiding this comment.
EnsureTrace takes op as an argument; the only other option would be adding a wrapper, but it would also add ~2 lines
It further decouples traceutil from the internal interface as now any (OTEL or other) tracing library, which relies on context, can be used. applyResult struct still needs to keep Trace field as "process raft request" doesn't propagate the caller's context (nor Trace ID). Signed-off-by: Aleksander Mistewicz <[email protected]>
AwesomePatrol
force-pushed
the
migration-to-otel-tracing-first-step
branch
from
1ec3f46 to
68968dd
Compare
| case r.Range != nil: | ||
| op = "Range" | ||
| ar.Resp, ar.Trace, ar.Err = a.applyV3.Range(r.Range) | ||
| ctx, trace := traceutil.EnsureTrace(context.Background(), a.lg, op) |
There was a problem hiding this comment.
Please move context.Background() to outside of switch to encourage other branches to also use it.
| case r.Range != nil: | ||
| op = "Range" | ||
| ar.Resp, ar.Trace, ar.Err = a.applyV3.Range(r.Range) | ||
| ctx, trace := traceutil.EnsureTrace(context.Background(), a.lg, op) |
There was a problem hiding this comment.
Calling EnsureTrace here means the calls in apply will be skipped. It means this changes names of operations and removes fields that are passed to EnsureTrace.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions. |
3 participants
Part of #12460.
context.Contextargument is added to replace returningtraceutil.Trace. This solves the problem of "merging" two traces and makes it easy to transition to nested spans possible in OpenTelemetry tracing.Some occurrences of
context.TODO()were also replaced with actual context.