Add LogRequest variable to config input #5197

animesh2049 · 2020-04-14T09:55:39Z

Fixes DGRAPH-1123

This PR adds support for logRequest config param. logRequest can have true/false values. True
value of logRequest enables logging of all requests coming to alphas. False value of logRequest
disables it.
Users will be able to dynamically change this param using graphql admin endpoint.
Sample query:

mutation {
  config(input: {logRequest: true}){
    response {
      code
      message
    }
  }
}

Internally we have added one more field in WorkerOptions(WorkerConfig) called as LogRequest
of int32 type. Value of this field is checked atomically upon every request arrival. If value of
LogRequest is 1, it results in logging of requests.

This change is

Docs Preview:

martinmr

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @animesh2049 and @manishrjain)

graphql/admin/config.go, line 43 at r1 (raw file):

	// It will be used like a boolean. We are keeping int32 because we want to
	// modify it using atomics(atomics doesn't have support for bool).
	LogRequest int32

From a user perspective, the mechanism to avoid using bools is confusing and is an internal limitation of golang to which they are now exposed. I think having a lock-protected boolean would be better. It could lead to contention but I don't think this endpoint will be called very often so that shouldn't be a problem.

animesh2049

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)

graphql/admin/config.go, line 43 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

From a user perspective, the mechanism to avoid using bools is confusing and is an internal limitation of golang to which they are now exposed. I think having a lock-protected boolean would be better. It could lead to contention but I don't think this endpoint will be called very often so that shouldn't be a problem.

Changed it to bool

…uests

pawanrawal

Reviewed 1 of 4 files at r3, 3 of 3 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @animesh2049, @manishrjain, and @martinmr)

graphql/admin/config.go, line 56 at r4 (raw file):

	if input.LogRequest != nil {
		if *input.LogRequest {
			atomic.StoreInt32(&x.WorkerConfig.LogRequest, 1)

0 can be false and 1 can be true.

x/config.go, line 87 at r3 (raw file):

	// BadgerKeyFile is the file containing the key used for encryption. Enterprise only feature.
	BadgerKeyFile string
	// LogRequest indicates whehter alpha should print all the requests

whether

ashish-goswami

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, and @pawanrawal)

graphql/admin/config.go, line 56 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

0 can be false and 1 can be true.

Done.

x/config.go, line 87 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

whether

Done.

manishrjain

Reviewed 1 of 4 files at r3, 1 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @animesh2049, @martinmr, and @pawanrawal)

graphql/admin/config.go, line 43 at r1 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Changed it to bool

Why is this a pointer, when LruMB is not?

graphql/admin/config.go, line 55 at r5 (raw file):

	// input.LogRequest will be nil, when it is not specified explicitly in config request.
	if input.LogRequest != nil {
		if *input.LogRequest {

No need for 2 level if. Can be one statement. But, again, don't think LogRequest should be a pointer to begin with.

manishrjain

Based on slack discussion, keep the logRequest bool pointer, but do address other comments.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @animesh2049, @martinmr, and @pawanrawal)

gja

Please move into a function, but otherwise looks ok

gja · 2020-05-13T10:05:04Z

graphql/admin/config.go

+	}
+
+	// input.LogRequest will be nil, when it is not specified explicitly in config request.
+	if input.LogRequest != nil {


workerConfig.SetLogRequest(true)

workerConfig.ShouldLogRequest()

gja · 2020-05-13T10:05:16Z

x/config.go

+	// Ideally LogRequest should be a bool value. But we are reading it using atomics across
+	// queries hence it has been kept as int32. LogRequest value 1 enables logging of requests
+	// coming to alphas and 0 disables it.
+	LogRequest int32


make this private, and use the getters / setters

Fixes: DGRAPH-1123 This PR adds support for logRequest config param. logRequest can have true/false values. True value of logRequest enables logging of all requests coming to alphas. False value of logRequest disables it. Users will be able to dynamically change this param using graphql admin endpoint. Sample query: mutation { config(input: {logRequest: true}){ response { code message } } } Internally we have added one more field in WorkerOptions(WorkerConfig) called as LogRequest of int32 type. Value of this field is checked atomically upon every request arrival. If value of LogRequest is 1, it results in logging of requests. Co-authored-by: Ashish Goswami <[email protected]>

Fixes: DGRAPH-1123 This PR adds support for logRequest config param. logRequest can have true/false values. True value of logRequest enables logging of all requests coming to alphas. False value of logRequest disables it. Users will be able to dynamically change this param using graphql admin endpoint. Sample query: mutation { config(input: {logRequest: true}){ response { code message } } } Internally we have added one more field in WorkerOptions(WorkerConfig) called as LogRequest of int32 type. Value of this field is checked atomically upon every request arrival. If value of LogRequest is 1, it results in logging of requests. Co-authored-by: Ashish Goswami <[email protected]> (cherry picked from commit 449c06b)

* Add LogRequest variable to config input (#5197) Fixes: DGRAPH-1123 This PR adds support for logRequest config param. logRequest can have true/false values. True value of logRequest enables logging of all requests coming to alphas. False value of logRequest disables it. Users will be able to dynamically change this param using graphql admin endpoint. Sample query: mutation { config(input: {logRequest: true}){ response { code message } } } Internally we have added one more field in WorkerOptions(WorkerConfig) called as LogRequest of int32 type. Value of this field is checked atomically upon every request arrival. If value of LogRequest is 1, it results in logging of requests. Co-authored-by: Ashish Goswami <[email protected]> (cherry picked from commit 449c06b) * fix(GraphQL): don't update cacheMb if not specified by user (GRAPHQL-888) (#7103) Fixes GRAPHQL-888. Previously, if you ran this request: ``` $ curl -H "Content-Type: application/json" http://localhost:8080/admin -d '{"query": "mutation {config(input: {logRequest: false}){response {code message}}}"}' ``` Alpha logs would also print this: ``` I1205 22:22:51.684693 2681396 middlewares.go:178] GraphQL admin mutation. Name = config I1205 22:22:51.684724 2681396 config.go:38] Got config update through GraphQL admin API I1205 22:22:51.684810 2681396 worker.go:138] Updating cacheMb to 0 ``` Indicating that cacheMb was also updated, even it wasn't specified in the request. This PR fixes this issue. (cherry picked from commit c03c327) # Conflicts: # graphql/admin/config.go Co-authored-by: Animesh Chandra Pathak <[email protected]> Co-authored-by: Abhimanyu Singh Gaur <[email protected]>

Add LogRequest variable to config input

9123539

animesh2049 requested review from manishrjain and a team as code owners April 14, 2020 09:55

martinmr suggested changes Apr 15, 2020

View reviewed changes

Change graphql input to Boolean

13fe655

animesh2049 commented Apr 16, 2020

View reviewed changes

ashish-goswami added 2 commits May 6, 2020 13:18

Merge remote-tracking branch 'origin/master' into animesh2049/log-req…

3554943

…uests

Add/modify comments

fc0c325

pawanrawal approved these changes May 8, 2020

View reviewed changes

Address review comments

74af8b5

ashish-goswami reviewed May 8, 2020

View reviewed changes

manishrjain suggested changes May 8, 2020

View reviewed changes

manishrjain reviewed May 8, 2020

View reviewed changes

gja approved these changes May 13, 2020

View reviewed changes

Address review comments

ca2086a

ashish-goswami merged commit 449c06b into master May 13, 2020

ashish-goswami deleted the animesh2049/log-requests branch May 13, 2020 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LogRequest variable to config input #5197

Add LogRequest variable to config input #5197

animesh2049 commented Apr 14, 2020 •

edited by lgalatin

Loading

martinmr left a comment

animesh2049 left a comment

pawanrawal left a comment

ashish-goswami left a comment

manishrjain left a comment

manishrjain left a comment

gja left a comment

gja May 13, 2020

gja May 13, 2020

Add LogRequest variable to config input #5197

Add LogRequest variable to config input #5197

Conversation

animesh2049 commented Apr 14, 2020 • edited by lgalatin Loading

martinmr left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

gja left a comment

Choose a reason for hiding this comment

gja May 13, 2020

Choose a reason for hiding this comment

gja May 13, 2020

Choose a reason for hiding this comment

animesh2049 commented Apr 14, 2020 •

edited by lgalatin

Loading