-
Notifications
You must be signed in to change notification settings - Fork 141
Add workflow.GetSpanContext and workflow.WithSpanContext API #1423
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 workflow.GetSpanContext and workflow.WithSpanContext API #1423
Conversation
} | ||
|
||
func spanFromContext(ctx Context) opentracing.SpanContext { | ||
func GetSpanContext(ctx Context) opentracing.SpanContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also expose contextWithSpan
or is there another way for users to add their existing context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm a bit concerned about the determinism if users start a new span inside the workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't have a clear story around replay traces. It wouldn't cause non-determinism though. Just weird/duplicate traces potentially.
Are you suggesting to expose the span via GetSpanContext
but don't let user set/override the span in the context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. It's still deterministics. Replay is going to cause issues. We will see multiple child spans if we allow users to set them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Sideeffect would work. But is span serializable or recoverable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. We can try and see. It sounds like we need to start a new span even for replays but let's discuss that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SpanContext is serializable and should be passed. With that said, we'll require users to start a short-lived new span just to modify spancontext. Please refer the examples in the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an integration test which works.
Detailed Description
Impact Analysis
Testing Plan
Rollout Plan
Why?
Users can now access baggage items in the workflow code.