-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
server: consistently log request_id at the same level #6244
Conversation
Beep boop! 🤖 Hey @lorenzo, thanks for your PR! One of my human friends will review this PR and get back to you as soon as possible. Stay awesome! 😎 |
✔️ Deploy preview for hasura-docs ready! 🔨 Explore the source changes: fbe0b8e 🔍 Inspect the deploy logs: https://app.netlify.com/sites/hasura-docs/deploys/60043e75df9a59000899e980 😎 Browse the preview: https://deploy-preview-6244--hasura-docs.netlify.app |
Hi @lorenzo Can you help me understand the problem? I think I'm missing something. You say:
But from the above sample log lines, I can see in the current shape:
So they're both at the same level right? I think I'm missing something, can you help me with it? |
@ecthiender Sorry, I messed up the log outputs in the description. I had copied twice the same example from the log output thinking they were different. I have changed the description, but I'll summarize here again:
After this change both will have: For backwards-compatibility, |
@ecthiender did that clarify the issue for you? |
@lorenzo yes it did. Thanks for the clarification. And sorry for the delay! |
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.
no changelog required
@lorenzo The docs would also require a change, correct? https://hasura.io/docs/1.0/graphql/core/deployment/logging.html#http-log-structure As this is user-facing, a changelog would be preferred. Something like: "add request_id to top level in http-log" |
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.
Just to confirm, will |
I don't think we can remove it without a major version bump. So it should not be removed anytime soon. |
@tirumaraiselvan jus to make sure, should I be changing the docs and adding a line to the changelog? |
@lorenzo Pls do! |
@tirumaraiselvan done. Can you please review again? |
Before this change, the server is logging requests with a different shape. - HTTPLog ```json { "type": "http-log", "timestamp": "2020-11-23T17:30:41.782+0100", "level": "info", "detail": { "operation": { "query_execution_time": 8.6201e-05, "user_vars": { "x-hasura-role": "admin" }, "request_id": "f579069a-23fc-4119-b2d8-4bab1a4e29e6", "response_size": 347, "request_read_time": 3.884e-06 }, "http_info": { "status": 200, "http_version": "HTTP/1.1", "url": "/foo", "ip": "127.0.0.1", "method": "GET", "content_encoding": "gzip" } } } ``` - QueryLog ```json { "detail": { "http_info" {...} "operation" { "query_execution_time": 0.001201056, "request_id": "e9bd9dd2-f5b3-4e26-9d88-9a39cff5acbb" ... } }, "level": "info", "timestamp": "2020-11-23T16:48:07.173+0000", "type": "http-log" } ``` Notice that the `request_id` is nested in different levels in both examples. This makes correlating the requests a bit more difficult than it should be. For instance, services like DataDog allow you to extract a json path as a facet that can be used in searches. Since the `request_id` is at a different level, it is not possible to correlate using the `request_id`. After this change, the `QueryLog` will look like ```json { "detail": { "http_info" {...} "operation" { "query_execution_time": 0.001201056, "request_id": "e9bd9dd2-f5b3-4e26-9d88-9a39cff5acbb" ... }, "request_id": "e9bd9dd2-f5b3-4e26-9d88-9a39cff5acbb" }, "level": "info", "timestamp": "2020-11-23T16:48:07.173+0000", "type": "http-log" } ``` The `request_id` is still present in the `operation` key for backwards compatibility.
Beep boop! 🤖 Awesome work @lorenzo! Your changes were merged successfully. All of us at Hasura ❤️ what you did. Thanks again 🤗 |
Description
Before this change, the server is logging requests with a different
shape.
Notice that the
request_id
is nested in different levels in bothexamples. This makes correlating the requests a bit more difficult than
it should be.
For instance, services like DataDog allow you to extract a json path as
a facet that can be used in searches. Since the
request_id
is at adifferent level, it is not possible to correlate using the
request_id
.After this change, the
HttpLog
will look likeThe
request_id
is still present in theoperation
key for backwardcompatibility.
Changelog
CHANGELOG.md
is updated with user-facing content relevant to this PR. If no changelog is required, then add theno-changelog-required
label.Affected components
Related Issues
Solution and Design
Steps to test and verify
Limitations, known bugs & workarounds
Server checklist
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
run_sql
auto manages the new metadata through schema diffing?run_sql
auto manages the definitions of metadata on renaming?export_metadata
/replace_metadata
supports the new metadata added?GraphQL
Breaking changes
[] No Breaking changes
There are breaking changes:
Metadata API
Existing
query
types:args
payload which is not backward compatibleJSON
schemaGraphQL API
Schema Generation:
NamedType
Schema Resolve:-
null
value for any input fieldsLogging
JSON
schema has changedtype
names have changed