Skip to content

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 16, 2025

I noticed that #57259 is still flaky after the latest changes. 4 failures in the last 30 days.

For some reason I thought it would be a good idea to sum the results from the total request counter. This doesn't make any sense 🤷 Maybe I was thinking of RPS counter when I did it. Total request counter is pre-summed so change the test back to test the latest value.

I've also made a change to improve logging and debugging the counter methods.

@JamesNK JamesNK added the area-hosting Includes Hosting label Jan 16, 2025
@JamesNK JamesNK requested a review from halter73 as a code owner January 16, 2025 03:06
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 23, 2025
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

LGTM overall!

Was the inaccurate totalRequestValues.WaitForSumValueAsync causing the flakiness or something else? I'd expect it to be a logical bug that would always cause the tests to fail instead of just being flaky.

@JamesNK
Copy link
Member Author

JamesNK commented Jan 24, 2025

Was the inaccurate totalRequestValues.WaitForSumValueAsync causing the flakiness or something else? I'd expect it to be a logical bug that would always cause the tests to fail instead of just being flaky.

Summing was always wrong for this value. But it is only a problem if it takes multiple seconds to get the right values. If the first set of values are correct then it doesn't have to sum and it passes.

I also much improved the logging for this test so if there are still problems then they should be more easier to observe.

@JamesNK JamesNK force-pushed the jamesnk/flaky-counters-tests-no-sum branch from 13d9f94 to fe0be7b Compare January 24, 2025 04:06
@JamesNK JamesNK enabled auto-merge (squash) January 24, 2025 04:07
@JamesNK JamesNK removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 24, 2025
@JamesNK
Copy link
Member Author

JamesNK commented Jan 25, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@JamesNK JamesNK merged commit 4af8275 into main Jan 25, 2025
27 checks passed
@JamesNK JamesNK deleted the jamesnk/flaky-counters-tests-no-sum branch January 25, 2025 11:35
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Jan 25, 2025
@danroth27 danroth27 added the task label Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-hosting Includes Hosting task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants