Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IdempotencyKey start span option #931

Merged
merged 3 commits into from
Oct 12, 2022

Conversation

jlegrone
Copy link
Contributor

What was changed

This PR adds a new IdempotencyKey field to TracerStartSpanOptions and sets this field on spans created from the WorkflowInboundInterceptor tracing implementation.

Why?

This option can be used to compute a deterministic span ID automatically for tracing implementations which support specifying an explicit span ID. This is being proposed as an alternative to the way we're computing the RunWorkflow span ID in #921.

This approach might also be used to provide a safe interface for handling custom spans created by end users in workflow code.

Checklist

  1. Closes

N/A

  1. How was this tested:

See new unit test for creating idempotency keys. There are no existing tracing implementations in tree which support explicit spans, but we may be able to add additional tests in #921 if this is merged.

  1. Any docs updates needed?

No

interceptor/tracing_interceptor.go Show resolved Hide resolved
return fmt.Sprintf("WorkflowInboundInterceptor:%s:%s:%s:%d",
t.info.Namespace,
t.info.WorkflowExecution.ID,
t.info.WorkflowExecution.RunID,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be noted that technically this value changes if a workflow is reset to a previous task. However, not only do we not appear to provide the "original run ID", but I think this is probably the correct behavior anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I like to think of workflow reset as a fancy continue-as-new. No problem with having a new trace for a new workflow run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can end up with orphaned spans this way if a wrapping workflow span crosses a reset point

"go.temporal.io/sdk/workflow"
)

func TestIdempotencyKeyGeneration(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this key is not used currently, I support no integration test with it. May want one to confirm counter is workflow-specific and replay safe though in the impl that does use it.

@jlegrone jlegrone marked this pull request as ready for review October 12, 2022 05:11
@jlegrone jlegrone requested a review from a team as a code owner October 12, 2022 05:11
@cretz cretz merged commit 14df12c into temporalio:master Oct 12, 2022
@jlegrone jlegrone deleted the jlegrone/trace-idempotency-key branch October 12, 2022 20:27
MarcosCela added a commit to MarcosCela/sdk-go that referenced this pull request Oct 19, 2022
Co-authored-by: Jacob LeGrone <jlegrone@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants