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

fixed bug in logging high complexity query rejections #581

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

irshadaj
Copy link
Contributor

@irshadaj irshadaj commented Apr 25, 2024

Description

A bug was introduced as part of #574 where we were attempting to log some fields that hadn't yet been populated from the buffer. This would result in nil pointer errors, panics, and/or empty logged fields. This manifested as the following error in the console when a cypher query exceeded the maximum complexity threshold:

Screenshot 2024-04-25 at 12 51 00 PM

The changes in this MR will populate all fields to be logged, before the log event is set up.

How Has This Been Tested?

Forced a query to exceed maximum complexity threshold by making the complexity analyzer (QueryComplexity function) return a high value

Screenshot 2024-04-25 at 1 00 51 PM

Ran a cypher query- this would trip the max complexity threshold and go into the high complexity log codepath. Error is shown at the bottom of the screen as expected.

Screenshot 2024-04-25 at 1 02 24 PM

Verified that the panic does not occur, and all expected fields are logged correctly

Screenshot 2024-04-25 at 12 57 53 PM

The other logging change sent out as part of the previous MR was related to neo4j timeouts. So I followed a similar strategy to force a neo4j timeout to occur, and verified that no nil pointers are thrown in that codepath.

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

@irshadaj irshadaj added bug Something isn't working work in progress This pull request is a work in progress and should not be merged labels Apr 25, 2024
@irshadaj irshadaj self-assigned this Apr 25, 2024
@irshadaj irshadaj removed the work in progress This pull request is a work in progress and should not be merged label Apr 25, 2024
@irshadaj irshadaj changed the title Draft: fixed bug in logging high complexity query rejections fixed bug in logging high complexity query rejections Apr 25, 2024
@mistahj67 mistahj67 self-requested a review April 25, 2024 18:44
Copy link
Contributor

@mistahj67 mistahj67 left a comment

Choose a reason for hiding this comment

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

I suspect there might be a way to have the single buffer but probably more work than it's worth.

Code LGTM and testing locally, no more panic, thanks for fixing!

@irshadaj irshadaj merged commit 4227b09 into main Apr 25, 2024
3 checks passed
@irshadaj irshadaj deleted the queryLogging_bug branch April 25, 2024 19:06
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants