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

feat(responsemanager): trace full messages via links to responses #325

Merged
merged 3 commits into from
Jan 12, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 5, 2022

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 the processUpdate span of the same response. Then in TestCancellationQueryInProgress we see the same thing but for an abortRequest triggered by the second message.

One possible change could be that we use this new message span as the parent to a response if no other parent gets inserted via the reuqestqueued context-modifying hook. So all response spans would have a message parent unless that hook is wired up by a consumer, as we do in TestIncomingQuery and I believe we'll be doing in datatransfer or further upstream. But I think this might miss the point of keeping responses as separate things that are independent of the individual messages. The message 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 .. ?

Copy link
Collaborator

@hannahhoward hannahhoward left a 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

@@ -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(
Copy link
Collaborator

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 :)

@hannahhoward
Copy link
Collaborator

And yes, we should keep the design as is

@rvagg rvagg force-pushed the rvagg/message-tracing branch 2 times, most recently from 8cac33c to 5c27818 Compare January 6, 2022 23:04
@hannahhoward
Copy link
Collaborator

@rvagg I merged your other PR first -- do you now want to make this processRequests for consistency?

@rvagg
Copy link
Member Author

rvagg commented Jan 10, 2022

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

@rvagg
Copy link
Member Author

rvagg commented Jan 10, 2022

@hannahhoward the failure on windows is a missing response(0)->executeTask(0)->processBlock(0)->loadBlock(0) trace in TestNetworkDisconnect. This doesn't come from the change in this PR. Do we need to remove all the response(X)->executeTask(Y)->* traces from that test, is it reasonable that they may be missing in some circumstances from this test?

@hannahhoward hannahhoward merged commit 022a166 into main Jan 12, 2022
rvagg added a commit that referenced this pull request Jan 13, 2022
* feat(responsemanager): trace full messages via links to responses

Fixes: #318

* chore(responsemanager): rename processRequests internals for consistency

* fix(responsemanager): make TestCancellationQueryInProgress less strict
rvagg added a commit that referenced this pull request Jan 14, 2022
* feat(responsemanager): trace full messages via links to responses

Fixes: #318

* chore(responsemanager): rename processRequests internals for consistency

* fix(responsemanager): make TestCancellationQueryInProgress less strict
@mvdan mvdan deleted the rvagg/message-tracing branch March 7, 2022 11:57
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.

Tracing: looking at how incoming messages move through the system - ResponseManager
2 participants