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 timeline logging to grpc client #392

Merged
merged 12 commits into from
Nov 11, 2020
Merged

Conversation

bkonyi
Copy link
Contributor

@bkonyi bkonyi commented Nov 3, 2020

isTimelineLoggingEnabled is used to turn on/off the logging. Note that only RPCs sent after enabling the logging will be recorded.

Continuation of https://github.com/grpc/grpc-dart/pull/336/files

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 3, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

Are responses added as metadata (instant events)? not as separate child timeline tasks like for HTTP events?

@bkonyi
Copy link
Contributor Author

bkonyi commented Nov 3, 2020

Are responses added as metadata (instant events)? not as separate child timeline tasks like for HTTP events?

Yes, that appears to be the case. I'm guessing we'd want child events for the responses?

@kenzieschmoll
Copy link

Are responses added as metadata (instant events)? not as separate child timeline tasks like for HTTP events?

Yes, that appears to be the case. I'm guessing we'd want child events for the responses?

IIRC, the http responses were added as separate TimelineTask events because it was possible that the response would come back after the request has completed. Is this possible for gRPC events? If so, we may want to use a similar implementation. But if not, then leaving the response as an instant event would also work.

@bkonyi
Copy link
Contributor Author

bkonyi commented Nov 4, 2020

Are responses added as metadata (instant events)? not as separate child timeline tasks like for HTTP events?

Yes, that appears to be the case. I'm guessing we'd want child events for the responses?

IIRC, the http responses were added as separate TimelineTask events because it was possible that the response would come back after the request has completed. Is this possible for gRPC events? If so, we may want to use a similar implementation. But if not, then leaving the response as an instant event would also work.

Yeah, I think it makes sense to be consistent, otherwise we'll be coming back to make that change later. I've updated to create a separate timeline task for the response with the request event as its parent.

Copy link
Member

@mraleph mraleph left a comment

Choose a reason for hiding this comment

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

LGTM with two comments.

lib/src/client/channel.dart Outdated Show resolved Hide resolved
lib/src/client/call.dart Outdated Show resolved Hide resolved
@bkonyi
Copy link
Contributor Author

bkonyi commented Nov 6, 2020

@mraleph addressed comments. Do you have write access to this repo? I'm unable to merge myself.

@mraleph
Copy link
Member

mraleph commented Nov 6, 2020

@bkonyi thanks I will merge on green once checks cycle. I will publish a version to pub early next week.

@bkonyi
Copy link
Contributor Author

bkonyi commented Nov 6, 2020

Thanks Slava!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants