这是indexloc提供的服务，不要输入任何密码
Skip to content

Allow request body to be logged if required even with successful queries #3529

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 16, 2019
Merged

Allow request body to be logged if required even with successful queries #3529

merged 2 commits into from
Dec 16, 2019

Conversation

@nizar-m
Copy link
Contributor

@nizar-m nizar-m commented Dec 13, 2019 •
edited
Loading

Description

LogHTTPSuccess now has the json parsed request body (if present) as an extra argument.
This is needed if we also want to log the request body with successful queries.

Affected components

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System
  • Tests
  • Other (list it)

Related Issues

Solution and Design

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No
  • Yes
    • Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

  • No
  • Yes
    • Does run_sql auto manages the new metadata through schema diffing?
      • Yes
      • Not required
    • Does run_sql auto manages the definitions of metadata on renaming?
      • Yes
      • Not required
    • Does export_metadata/replace_metadata supports the new metadata added?
      • Yes
      • Not required

GraphQL

  • No new GraphQL schema is generated
  • New GraphQL schema is being generated:
    • New types and typenames are correlated

Breaking changes

  • No Breaking changes

  • There are breaking changes:

    1. Metadata API

      Existing query types:

      • Modify args payload which is not backward compatible
      • Behavioural change of the API
      • Change in response JSON schema
      • Change in error code

    2. GraphQL API

      Schema Generation:

      • Change in any NamedType
      • Change in table field names

      Schema Resolve:-

      • Change in treatment of null value for any input fields

    3. Logging

      • Log JSON schema has changed
      • Log type names have changed

Steps to test and verify

Limitations, known bugs & workarounds

@nizar-m nizar-m requested a review from lexi-lambda as a code owner December 13, 2019 05:58
@netlify
Copy link

netlify bot commented Dec 13, 2019 •
edited
Loading

Deploy preview for hasura-docs ready!

Built with commit 039cc9b

https://deploy-preview-3529--hasura-docs.netlify.com

@nizar-m nizar-m changed the title Allow request body to be logged even with successful queries Allow request body to be logged if required even with successful queries Dec 13, 2019
@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 13, 2019

Review app for commit c638941 deployed to Heroku: https://hge-ci-pull-3529.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3529-c6389413

@lexi-lambda lexi-lambda added the c/server Related to server label Dec 14, 2019
lexi-lambda
lexi-lambda reviewed Dec 14, 2019
View reviewed changes
Copy link
Contributor

@lexi-lambda lexi-lambda left a comment

Choose a reason for hiding this comment

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

This seems like it could result in pretty noisy logs, right? I don’t really have a fundamental problem with this change, but does it make sense to put this behind a flag or something? Most servers don’t log every single request payload!

@nizar-m
Copy link
Contributor Author

nizar-m commented Dec 16, 2019

This seems like it could result in pretty noisy logs, right? I don’t really have a fundamental problem with this change, but does it make sense to put this behind a flag or something? Most servers don’t log every single request payload!

This maybe useful in some cases. For example you may want to always log the request body of metadata queries.

lexi-lambda
lexi-lambda approved these changes Dec 16, 2019
View reviewed changes
Copy link
Contributor

@lexi-lambda lexi-lambda left a comment

Choose a reason for hiding this comment

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

This maybe useful in some cases. For example you may want to always log the request body of metadata queries.

Ah, I see, I think I misinterpreted the diff at first glance—this doesn’t actually change the behavior at all, it just exposes the information via the HttpLog class. That seems perfectly reasonable to me, so that’s my mistake.

@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 16, 2019

Review app for commit 039cc9b deployed to Heroku: https://hge-ci-pull-3529.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3529-039cc9b3

@lexi-lambda lexi-lambda merged commit c0038a7 into hasura:master Dec 16, 2019
@hasura-bot
Copy link
Contributor

hasura-bot commented Dec 16, 2019

Review app https://hge-ci-pull-3529.herokuapp.com is deleted

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
@nizar-m @polRk
Allow request body to be logged even with successful queries (hasura#…
240b51c 
…3529)
@sumodgeorge sumodgeorge mentioned this pull request Nov 28, 2023
Open
@Allymahmoud Allymahmoud mentioned this pull request Nov 28, 2023
Open
@ptakpatryk ptakpatryk mentioned this pull request Dec 19, 2023
Open
This was referenced Apr 6, 2024
Open
Open
This was referenced Apr 6, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lexi-lambda lexi-lambda lexi-lambda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c/server Related to server

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nizar-m @hasura-bot @lexi-lambda