-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: add metrics cost to grpc and http header #6148
Conversation
Signed-off-by: பாலாஜி <[email protected]>
Signed-off-by: Balaji <[email protected]>
Signed-off-by: Balaji <[email protected]>
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.
Small feedback, but looks good
dgraph/cmd/alpha/http_test.go
Outdated
@@ -196,6 +196,37 @@ func queryWithTs(queryText, contentType, debug string, ts uint64) (string, uint6 | |||
return string(output), startTs, err | |||
} | |||
|
|||
func queryWithTsForCost(queryText, contentType, debug string, ts uint64) (string, |
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.
rather than making this a new function, you should probably pass options to queryWithTs(). Or if you don't want to do update all the callers.
type validationOpts struct {
ResponseCallback func()
}
func _queryWithTs(..., opts *validationOpts) {
...
context := map[string]interface{}
if opt.ResponseCallback {
err := opt.ResponseCallback(context, response)
}
return ..., context, error
}
func Test...() {
...
queryWithTs(..., &callbackOptions{ResponseCallback: func() { // put cost into context }})
}
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.
I've made it to return resp instead of passing the validation function.
Signed-off-by: பாலாஜி <[email protected]>
Signed-off-by: பாலாஜி <[email protected]>
Signed-off-by: பாலாஜி <[email protected]>
x/x.go
Outdated
@@ -146,6 +146,7 @@ const ( | |||
AccessControlAllowedHeaders = "X-Dgraph-AccessToken, " + | |||
"Content-Type, Content-Length, Accept-Encoding, Cache-Control, " + | |||
"X-CSRF-Token, X-Auth-Token, X-Requested-With" | |||
DgraphCostHeader = "numUids" |
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.
Oh sorry, I should have been explicit 🙈.. Let's keep it similar to graphql.
Dgraph-TouchedUids
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.
Reviewed 1 of 4 files at r1, 1 of 2 files at r2, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @balajijinnah, @gja, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)
dgraph/cmd/alpha/http_test.go, line 199 at r5 (raw file):
} func queryWithTsForResp(queryText, contentType, debug string, ts uint64) (string,
Add a comment for these functions so it's clearer what the difference is to the other function with a similar name.
Signed-off-by: பாலாஜி <[email protected]>
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.
Unit Tests for the Metric?
Reviewed 1 of 4 files at r1, 1 of 2 files at r2, 1 of 1 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @balajijinnah, @gja, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)
@darkn3rd I've added integration test. That should be enough. |
This change is