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

repo-updater: Fix update queue length metric #17308

Merged
merged 1 commit into from
Jan 15, 2021

Conversation

ryanslade
Copy link
Contributor

Previously we used calls to Push and Pop which don't appear to be
correct as we observed negative length values.

Instead, we'll monitor the length of the heap itself.

Previously we used calls to Push and Pop which don't appear to be
correct as we observed negative length values.

Instead, we'll monitor the length of the heap itself.
@ryanslade ryanslade added this to the Cloud 2021-01-13 milestone Jan 15, 2021
@ryanslade ryanslade requested a review from a team January 15, 2021 09:42
@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #17308 (84db64b) into main (08f0293) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main   #17308   +/-   ##
=======================================
  Coverage   52.14%   52.14%           
=======================================
  Files        1712     1712           
  Lines       85441    85441           
  Branches     7596     7596           
=======================================
+ Hits        44554    44555    +1     
- Misses      36972    36973    +1     
+ Partials     3915     3913    -2     
Flag Coverage Δ *Carryforward flag
go 51.22% <100.00%> (+<0.01%) ⬆️
integration 30.59% <ø> (ø) Carriedforward from 08f0293
storybook 30.23% <ø> (ø) Carriedforward from 08f0293
typescript 54.39% <ø> (ø) Carriedforward from 08f0293
unit 34.87% <ø> (ø) Carriedforward from 08f0293

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
internal/repos/scheduler.go 65.34% <100.00%> (ø)
...nal/campaigns/resolvers/changeset_apply_preview.go 58.97% <0.00%> (-0.86%) ⬇️
.../internal/codeintel/resolvers/graphql/locations.go 85.56% <0.00%> (+2.06%) ⬆️

Copy link
Contributor

@arussellsaw arussellsaw left a comment

Choose a reason for hiding this comment

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

LGTM, but observing negative values seems like we're either panicking & recoivering or not calling Push & Pop correctly, do you have an idea of why we saw negative values?

@ryanslade
Copy link
Contributor Author

I'm not sure. Push and Pop are part of the heap interface and we don't call them ourselves so I have to assume they're being called correctly.

The value seems to consistently drop over time but at some point it jumps down to zero and then continues "dropping" below zero:

Screenshot 2021-01-15 at 12 37 29

We do also set the value to zero when we stop reading from the update queue but this only called once in our code when the entire scheduler is shutting down.

I'll continue to dig into this as it's also strange how large the update queue is

@ryanslade ryanslade merged commit c59ea3c into main Jan 15, 2021
@ryanslade ryanslade deleted the fix-update-queue-length-metric branch January 15, 2021 10:40
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.

None yet

3 participants