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(GraphQL): Log query along with the panic #7638

Merged
merged 3 commits into from
Mar 24, 2021

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented Mar 23, 2021

This PR adds functionality to log the query and mutations which cause panic in PanicHandler.

Testing:
Tested locally.


This change is Reviewable

@vmrajas vmrajas requested a review from pawanrawal as a code owner March 23, 2021 15:04
@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Mar 23, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur 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: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @pawanrawal and @vmrajas)


graphql/resolve/resolver.go, line 476 at r1 (raw file):

Quoted 13 lines of code…
	resp := &schema.Response{
		Extensions: &schema.Extensions{
			Tracing: &schema.Trace{
				Version:   1,
				StartTime: startTime.Format(time.RFC3339Nano),
			},
		},
	}
	defer func() {
		endTime := time.Now()
		resp.Extensions.Tracing.EndTime = endTime.Format(time.RFC3339Nano)
		resp.Extensions.Tracing.Duration = endTime.Sub(startTime).Nanoseconds()
	}()

let's bring this back here


graphql/resolve/resolver.go, line 480 at r1 (raw file):

return schema.ErrorResponse(err)

I am realizing that this won't return tracing information in the response.
So, we should do this here:

resp.Errors = schema.AsGQLErrors(err)
return

graphql/resolve/resolver.go, line 486 at r1 (raw file):

return schema.ErrorResponse(err)
resp.Errors = schema.AsGQLErrors(err)
return

graphql/resolve/resolver.go, line 570 at r1 (raw file):

Quoted 4 lines of code…
		api.PanicHandler(
			func(err error) {
				resp.Errors = schema.AsGQLErrors(schema.AppendGQLErrs(resp.Errors, err))
			}, gqlReq.Query)

we can put this as the first line in the already existing defer func() above. That way any bad thing inside Resolve() would be caught here itself, and the panic handler in recoveryHandler would be responsible for a very limited set of things.

Copy link
Contributor Author

@vmrajas vmrajas 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: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur and @pawanrawal)


graphql/resolve/resolver.go, line 476 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
	resp := &schema.Response{
		Extensions: &schema.Extensions{
			Tracing: &schema.Trace{
				Version:   1,
				StartTime: startTime.Format(time.RFC3339Nano),
			},
		},
	}
	defer func() {
		endTime := time.Now()
		resp.Extensions.Tracing.EndTime = endTime.Format(time.RFC3339Nano)
		resp.Extensions.Tracing.Duration = endTime.Sub(startTime).Nanoseconds()
	}()

let's bring this back here

Done.


graphql/resolve/resolver.go, line 480 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
return schema.ErrorResponse(err)

I am realizing that this won't return tracing information in the response.
So, we should do this here:

resp.Errors = schema.AsGQLErrors(err)
return

Yes, it was also the case before this PR. I will fix this with this PR itself.


graphql/resolve/resolver.go, line 486 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
return schema.ErrorResponse(err)
resp.Errors = schema.AsGQLErrors(err)
return

Done.


graphql/resolve/resolver.go, line 570 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
		api.PanicHandler(
			func(err error) {
				resp.Errors = schema.AsGQLErrors(schema.AppendGQLErrs(resp.Errors, err))
			}, gqlReq.Query)

we can put this as the first line in the already existing defer func() above. That way any bad thing inside Resolve() would be caught here itself, and the panic handler in recoveryHandler would be responsible for a very limited set of things.

Done.

@vmrajas vmrajas force-pushed the rajas/GRAPHQL-1062 branch from ee6dd6c to 31d786f Compare March 24, 2021 09:06
@vmrajas vmrajas merged commit 071d5de into release/v21.03 Mar 24, 2021
@vmrajas vmrajas deleted the rajas/GRAPHQL-1062 branch March 24, 2021 09:19
vmrajas added a commit that referenced this pull request Mar 24, 2021
* Log query along with panic

* Log mutations using panic handler

* Address Abhimanyu's comments

(cherry picked from commit 071d5de)
vmrajas added a commit that referenced this pull request Mar 24, 2021
* Log query along with panic

* Log mutations using panic handler

* Address Abhimanyu's comments

(cherry picked from commit 071d5de)
aman-bansal pushed a commit that referenced this pull request Mar 25, 2021
* Log query along with panic

* Log mutations using panic handler

* Address Abhimanyu's comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants