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

Adding Transport client request life cycle timeline on diagnostics #7500

Merged

Conversation

simplynaveen20
Copy link
Member

@simplynaveen20 simplynaveen20 commented Jan 16, 2020

This will add timeline for various stage of transport client request on diagnostics, both on Direct mode(Rntbd) or in Gateway (ReactorNettyHttp).
Below how it looks on diagnostics string

Rntbd protocol -

"transportRequestTimeline": [
{
"eventName": "created",
"startTime": "2020-02-04T14:54:59.488-05:00",
"durationInMicroSec": "51000"
},
{
"eventName": "queued",
"startTime": "2020-02-04T14:54:59.539-05:00",
"durationInMicroSec": "266000"
},
{
"eventName": "pipelined",
"startTime": "2020-02-04T14:54:59.805-05:00",
"durationInMicroSec": "9990000"
},
{
"eventName": "transitTime",
"startTime": "2020-02-04T14:55:09.795-05:00",
"durationInMicroSec": "50000"
},
{
"eventName": "received",
"startTime": "2020-02-04T14:55:09.845-05:00",
"durationInMicroSec": "20000"
},
{
"eventName": "completed",
"startTime": "2020-02-04T14:55:09.865-05:00",
"durationInMicroSec": "0"
}
]

Http protocol -

"transportRequestTimeline": [
{
"eventName": "connectionCreated",
"startTime": "2020-02-04T14:53:47.932-05:00",
"durationInMicroSec": "78000"
},
{
"eventName": "connectionConfigured",
"startTime": "2020-02-04T14:53:48.010-05:00",
"durationInMicroSec": "1000"
},
{
"eventName": "requestSent",
"startTime": "2020-02-04T14:53:48.011-05:00",
"durationInMicroSec": "2000"
},
{
"eventName": "transitTime",
"startTime": "2020-02-04T14:53:48.013-05:00",
"durationInMicroSec": "51000"
},
{
"eventName": "received",
"startTime": "2020-02-04T14:53:48.064-05:00",
"durationInMicroSec": "1000"
}
]

@moderakh
Copy link
Contributor

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

  1. Could you please share a sample for diagnostics string toString() at the end?
  2. Could you please run ReadThroughput workload on the azure-vm to measure what's the impact of the perf?

@simplynaveen20
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

Please merge feature/cosmos/v4 to your branch.

  • High level this PR looks good to me.
  • Please fix the CIs, and let's run perf,

also I comment on the merging ReactorNettyRequestRecord into HttpResponse. what are your thoughts on that.

@simplynaveen20
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

LGTM.

Just it is worth if @David-Noble-at-work also could take a look too. as David did some work in both rntbd and diagnostics his input will be valuable.

Copy link
Member

@kushagraThapar kushagraThapar left a comment

Choose a reason for hiding this comment

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

LGTM, let' wait for @David-Noble-at-work 's review on this.

@simplynaveen20 simplynaveen20 merged commit 44ba79d into Azure:feature/cosmos/v4 Feb 10, 2020
@simplynaveen20
Copy link
Member Author

issue have been open as an extention to this PR #8035

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.

5 participants