Skip to content

Conversation

@JPrevost
Copy link
Member

@JPrevost JPrevost commented Jan 25, 2023

Why are these changes being introduced:

  • understanding which fields, including deprecated fields and arguments, are being used will help us better understand if changes we are proposing will affect actual users

Relevant ticket(s):

How does this address that need:

Document any side effects to this change:

  • Not a side effect, but we may need to adjust how/where we log to make this date more usable. Getting this into our standard logging pipeline seemed like a good place to start though and it will be easy to change the behavior of the result method to do something different once we decide what we need.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

Why are these changes being introduced:

* understanding which fields, including deprecated fields and arguments,
  are being used will help us better understand if changes we are proposing
  will affect actual users

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/TIMX-172

How does this address that need:

* Uses Ahead-of-Time AST Analysis features of graphql-ruby and their
  provided FieldUsage to log useful information via Rails.logger
* https://graphql-ruby.org/queries/ast_analysis.html
* https://graphql-ruby.org/api-doc/2.0.16/GraphQL/Analysis/AST/FieldUsage.html

Document any side effects to this change:

* Not a side effect, but we may need to adjust how/where we log to make
  this date more usable. Getting this into our standard logging pipeline
  seemed like a good place to start though and it will be easy to change
  the behavior of the `result` method to do something different once we
  decide what we need.
@JPrevost JPrevost force-pushed the timx-172-log-field-usage branch from 33fa45f to fd1627a Compare January 25, 2023 21:02
@JPrevost JPrevost temporarily deployed to timdex-pr-640 January 25, 2023 21:02 Inactive
@JPrevost
Copy link
Member Author

A query such as

{
  search(searchterm: "popcorn") {
    hits
    records {
      title
      identifier
      highlight {
        matchedField
        matchedPhrases
      }
    }
  }
}

In the PR build or in a local app running this branch should show the data being logged (there is a lot of noise as well but if you search you should see the lines added here but showing all fields as well as separately deprecated fields). We currently don't have any deprecated arguments, but I have a separate ticket that will deprecate source. This logging is prepared for that but in theory as-is nothing will ever be logged under used_deprecated_arguments. Soon though...

@JPrevost JPrevost requested a review from jazairi January 25, 2023 21:14
@jazairi jazairi self-assigned this Jan 25, 2023
Copy link
Contributor

@jazairi jazairi left a comment

Choose a reason for hiding this comment

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

This is working locally as expected. Cool feature! :shipit:

@JPrevost JPrevost merged commit 7b1aef7 into main Jan 26, 2023
@JPrevost JPrevost deleted the timx-172-log-field-usage branch January 26, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants