-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat(responsemanager): trace full messages via links to responses #325
Conversation
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.
Do you mind changing the name of message span to requestMessage to mirror the responseMessage on the other side and make it less generic? Otherwise LGTM
responsemanager/server.go
Outdated
@@ -166,25 +174,36 @@ func (rm *ResponseManager) abortRequest(p peer.ID, requestID graphsync.RequestID | |||
} | |||
|
|||
func (rm *ResponseManager) processRequests(p peer.ID, requests []gsmsg.GraphSyncRequest) { | |||
ctx, messageSpan := otel.Tracer("graphsync").Start(rm.ctx, "message", trace.WithAttributes( |
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.
rename requestMessage to match corresponding responseMessage on the other side (also cause I'm about to submit a PR that conflicts :)
And yes, we should keep the design as is |
8cac33c
to
5c27818
Compare
@rvagg I merged your other PR first -- do you now want to make this processRequests for consistency? |
5c27818
to
0a6c17b
Compare
yes, I didn't even think about the responsemanager when I did that one, but it's done here now, and ooof that was a painful rebase but this is now up to date as well |
b07d257
to
04bf81f
Compare
@hannahhoward the failure on windows is a missing |
* feat(responsemanager): trace full messages via links to responses Fixes: #318 * chore(responsemanager): rename processRequests internals for consistency * fix(responsemanager): make TestCancellationQueryInProgress less strict
* feat(responsemanager): trace full messages via links to responses Fixes: #318 * chore(responsemanager): rename processRequests internals for consistency * fix(responsemanager): make TestCancellationQueryInProgress less strict
Fixes: #318
Probably the most interesting part of this is seeing two separate messages do different things to a single response. In
TestPauseResumeViaUpdate
we see the first message linked to the start of a response and the second message get linked to theprocessUpdate
span of the same response. Then inTestCancellationQueryInProgress
we see the same thing but for anabortRequest
triggered by the second message.One possible change could be that we use this new
message
span as the parent to aresponse
if no other parent gets inserted via the reuqestqueued context-modifying hook. So allresponse
spans would have amessage
parent unless that hook is wired up by a consumer, as we do inTestIncomingQuery
and I believe we'll be doing in datatransfer or further upstream. But I think this might miss the point of keepingresponse
s as separate things that are independent of the individual messages. Themessage
span isn't going to live longer than the synchronous call here but I don't think it would matter to a consuming API if they stacked up, but it does suggest that we're looking at these things in different ways so we should probably keep them separate .. ?