Propogate context for better span correlation of recording viewing #49142
Conversation
|
Trace prior to this change: Trace with this change: |
rosstimothy
force-pushed
the
tross/playback_tracing
branch
from
9ee8754 to
76107ea
Compare
Traces generated from playing back session recordings were incomplete due to the correct context not being used at various levels of the events and player code. This attempts to rectify that by setting the correct context.Context along the way.
rosstimothy
force-pushed
the
tross/playback_tracing
branch
from
76107ea to
0431952
Compare
rosstimothy
added
no-changelog
github-actions
bot
added
audit-log
| ctx := context.Background() | ||
| if cfg.Context != nil { | ||
| ctx = cfg.Context | ||
| } | ||
|
|
There was a problem hiding this comment.
Isn't it better to pass it as a first argument to New? I get New shouldn't get context, but it shouldn't start goroutines either. At least it'd be clearer it does something beyond creation. Or while changing the signature (adding a new argument) possibly renaming to Start or something is an option.
There was a problem hiding this comment.
I agree the better approach would probably be to separate creating the Player from starting the playback. However, I wasn't sure that making such changes here was wise.
There was a problem hiding this comment.
I thought Start(ctx context.Context, cfg *Config) (*Player, error) could both create and start the Player (same as New does now). Would it be a big change to replace New with that?
There was a problem hiding this comment.
I'm not really sure that's much better. I think I'd prefer splitting initialization of the player and starting the play back, though that has idempotency concerns.
There was a problem hiding this comment.
I agree splitting this would be ideal, but I think we don't share the same concern. My worry is that there is a chance someone will forget to pass the context again. But let's not drill this any longer.
|
@rosstimothy See the table below for backport results.
|
Traces generated from playing back session recordings were incomplete due to the correct context not being used at various levels of the events and player code. This attempts to rectify that by setting the correct context.Context along the way.