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

Fix how query stats are calculated. #3711

Merged
merged 2 commits into from
Aug 9, 2019
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Jul 23, 2019

  • Add stat for how long it takes to retrieve to get a timestamp from
    zero.
  • Simplify encoding_ns logic to not depend on the rest of the stats as
    that might lead to bugs and is overall pretty brittle.
  • Include time to process type and schema queries in the processing
    time.

This change is Reviewable

- Add stat for how long it takes to retrieve to get a timestamp from
zero.
- Simplify encoding_ns logic to not depend on the rest of the stats as
that might lead to bugs and is overall pretty brittle.
- Include time to process type and schema queries in the processing
time.
@martinmr martinmr requested review from manishrjain and a team as code owners July 23, 2019 18:57
@martinmr
Copy link
Contributor Author

This depends on dgraph-io/dgo#74. Once that's merged I will properly vendor the changes into Dgraph. For now I just copied the protobuf file to allow my changes to compile.

@martinmr
Copy link
Contributor Author

Related to #3690

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

I haven't looked too deeply, but looks sorta alright from a high level. @gitlw can do a deeper review.

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @golangcibot)

@martinmr martinmr requested review from gitlw and a team August 6, 2019 17:11
Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @golangcibot and @martinmr)


query/query.go, line 2783 at r1 (raw file):

		}
	}
	req.Latency.Processing += time.Since(schemaProcessingStart)

Right now, the Latency.Processing includes two parts, the one inside ProcessQuery, and this new one you are adding here. Maybe consolidate them into one that starts the timer at the beginning of this function.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @golangcibot)


query/query.go, line 2783 at r1 (raw file):

Previously, gitlw (Lucas Wang) wrote…

Right now, the Latency.Processing includes two parts, the one inside ProcessQuery, and this new one you are adding here. Maybe consolidate them into one that starts the timer at the beginning of this function.

That wouldn't work, the parsing time is calculated inside ProcessQuery and then the processing time is started.

@martinmr martinmr requested a review from gitlw August 9, 2019 17:22
Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @golangcibot)

@martinmr martinmr merged commit 5e8c5e1 into master Aug 9, 2019
@martinmr martinmr deleted the martinmr/fix-query-stats branch August 9, 2019 19:26
danielmai pushed a commit that referenced this pull request Aug 9, 2019
- Add stat for how long it takes to retrieve to get a timestamp from
zero.
- Simplify encoding_ns logic to not depend on the rest of the stats as
that might lead to bugs and is overall pretty brittle.
- Include time to process type and schema queries in the processing
time.

(cherry picked from commit 5e8c5e1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants