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

symbols: canceling ParseRequest does not spam logs #61719

Merged
merged 1 commit into from Apr 10, 2024
Merged

Conversation

keegancsmith
Copy link
Member

We are seeing lots of log spam related to cancelation of parse requests. We haven't fully tracked this down yet, but the behaviour of spamming logs in this case hides the real issue.

This adjusts the implementation in two places:

  • We introduce a parseCanceled metric so we can still track this. This is used instead of parseFailed in case of cancelation. In this case we also do not emit the "Closing failed parser" even though we are. This is due to log spam.
  • We introduce an ErrorFilter just for handleParseRequest that will elide Canceled from the logs. This is called per file. Note: we keep the error logging for canceled in the high level Parse handler which runs across all files.

Test Plan: go test. Unfortunetly that is as good as it gets for now, as I investigate the root cause further I will test this code path (and only merge once I have manually triggered and tested this code path).

@keegancsmith keegancsmith requested a review from a team April 9, 2024 13:07
@cla-bot cla-bot bot added the cla-signed label Apr 9, 2024
@github-actions github-actions bot added team/product-platform team/search-platform Issues owned by the search platform team labels Apr 9, 2024
We are seeing lots of log spam related to cancelation of parse requests.
We haven't fully tracked this down yet, but the behaviour of spamming
logs in this case hides the real issue.

This adjusts the implementation in two places:
- We introduce a parseCanceled metric so we can still track this. This
  is used instead of parseFailed in case of cancelation. In this case we
  also do not emit the "Closing failed parser" even though we are. This
  is due to log spam.
- We introduce an ErrorFilter just for handleParseRequest that will
  elide Canceled from the logs. This is called per file. Note: we keep
  the error logging for canceled in the high level Parse handler which
  runs across all files.

Test Plan: go test. Unfortunetly that is as good as it gets for now, as
I investigate the root cause further I will test this code path (and
only merge once I have manually triggered and tested this code path).
Copy link
Member

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Hopefully this will get us closer to root causing the cancellations!

@keegancsmith keegancsmith merged commit ae59c69 into main Apr 10, 2024
11 checks passed
@keegancsmith keegancsmith deleted the k/symbols-log branch April 10, 2024 06:29
sourcegraph-release-guild-bot pushed a commit that referenced this pull request Apr 11, 2024
We are seeing lots of log spam related to cancelation of parse requests.
We haven't fully tracked this down yet, but the behaviour of spamming
logs in this case hides the real issue.

This adjusts the implementation in two places:
- We introduce a parseCanceled metric so we can still track this. This
  is used instead of parseFailed in case of cancelation. In this case we
  also do not emit the "Closing failed parser" even though we are. This
  is due to log spam.
- We introduce an ErrorFilter just for handleParseRequest that will
  elide Canceled from the logs. This is called per file. Note: we keep
  the error logging for canceled in the high level Parse handler which
  runs across all files.

Test Plan: go test. Unfortunetly that is as good as it gets for now, as
I investigate the root cause further I will test this code path (and
only merge once I have manually triggered and tested this code path).

(cherry picked from commit ae59c69)
keegancsmith added a commit that referenced this pull request Apr 12, 2024
#61772)

symbols: canceling ParseRequest does not spam logs (#61719)

We are seeing lots of log spam related to cancelation of parse requests.
We haven't fully tracked this down yet, but the behaviour of spamming
logs in this case hides the real issue.

This adjusts the implementation in two places:
- We introduce a parseCanceled metric so we can still track this. This
  is used instead of parseFailed in case of cancelation. In this case we
  also do not emit the "Closing failed parser" even though we are. This
  is due to log spam.
- We introduce an ErrorFilter just for handleParseRequest that will
  elide Canceled from the logs. This is called per file. Note: we keep
  the error logging for canceled in the high level Parse handler which
  runs across all files.

Test Plan: go test. Unfortunetly that is as good as it gets for now, as
I investigate the root cause further I will test this code path (and
only merge once I have manually triggered and tested this code path).

(cherry picked from commit ae59c69)

Co-authored-by: Keegan Carruthers-Smith <keegan.csmith@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants