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

refactor(GraphQL): Rename logRequest name and its resolver. #8388

Merged
merged 1 commit into from
Nov 12, 2022

Conversation

MichelDiz
Copy link
Contributor

@MichelDiz MichelDiz commented Oct 25, 2022

Problem

In order to add GraphQL Query Log(#8305) we need to rename the current DQL query log resolver and config.

Solution

Rename: logRequest => logDQLRequest

Important change:

--- glog.Infof("Got a query: %+v", req.req)
+++ glog.Infof("Got a DQL query: %+v", req.req)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 37.18% when pulling 2b4aeba on micheldiz/renameLogReq into c36206a on main.

@skrdgraph
Copy link
Contributor

I have reviewed this change and it looks okay.

@matthewmcneely could you also review this please?

@matthewmcneely
Copy link
Member

@MichelDiz So when logging is turned on via the /admin endpoint, how does the GraphQL query get logged? Is that already in the codebase?

@MichelDiz
Copy link
Contributor Author

MichelDiz commented Nov 2, 2022

@matthewmcneely Right now I'm first renaming this resolver, and then I'll implement the query log for GraphQL. There is no log for GraphQL yet. I'm thinking of putting it to Mutations too. To be able to view DQL/RDF mutations in the logs optionally.

@matthewmcneely
Copy link
Member

@MichelDiz Ah OK, so this is just laying the foundation. One interesting aspect of when GraphQL logging is enabled is that you'll be able to view both the GraphQL and then the generated DQL query.

One thought related to that, you might consider tagging the log entry with a identifier that maps to the query context id, that way the GraphQL and generated DQL query could be compared.

@skrdgraph
Copy link
Contributor

I really like the above idea suggested by @matthewmcneely. The identifier facilitates poor many tracing in the logs.

Thinking a little larger here (may be a dumb question), doesn't the tracing setup we have help us here? https://dgraph.io/docs/deploy/tracing/#setting-up-multiple-dgraph-clusters-with-jaeger ..

Copy link
Contributor

@skrdgraph skrdgraph left a comment

Choose a reason for hiding this comment

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

This PR looks good. 👍

@MichelDiz
Copy link
Contributor Author

I will see if it will be necessary for an identifier. As I recall both queries (Graphql and DQL) appear at the same time. One pasted with the other at runtime. But I'll test it anyway.

@skrdgraph It can help the user to understand which query was, for example, the cause of a crash. But Jaeger can help identify at another atomic level. But this log will not appear in Jaeger. However, yes both together can be a powerful see the big picture. Jaeger does the predicate-level trace. The Query log would help you know what to look for in Jaeger for example.

BTW, Jaeger is a pretty cool tool. You can understand a lot on how Dgraph execute things. It shows each call in details. So you can have an idea of it.

@MichelDiz MichelDiz merged commit bc5f584 into main Nov 12, 2022
@MichelDiz MichelDiz deleted the micheldiz/renameLogReq branch November 12, 2022 16:35
all-seeing-code pushed a commit that referenced this pull request Dec 12, 2022
In order to add GraphQL Query Log(#8305) we need to rename the current
DQL query log resolver and config.

> Rename: logRequest => logDQLRequest

Important change:

```diff
--- glog.Infof("Got a query: %+v", req.req)
+++ glog.Infof("Got a DQL query: %+v", req.req)
```

(cherry picked from commit bc5f584)
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