-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Deduplicate configuration cache report problems #36047
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
03fa21f to
0f1ecef
Compare
| withProblem("Task `:problems` of type `BrokenTask`: cannot serialize object of type 'org.gradle.api.internal.project.DefaultProject', a subtype of 'org.gradle.api.Project', as these are not supported with the configuration cache.") | ||
| totalProblemsCount = 6 | ||
| problemsWithStackTraceCount = 2 | ||
| problemsWithStackTraceCount = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are unique counts now
| addNotReportedDegradingTasks() | ||
| addDegradingFeatures() | ||
| val summary = summarizer.get() | ||
| val hasNoProblemsForConsole = summary.reportableProblemCount == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got confused by reportableProblemCount - it really means a count of problems we report to the console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry 😄
0f1ecef to
0b30add
Compare
| val lock = ReentrantLock() | ||
|
|
||
| fun get(): Summary = lock.withLock { | ||
| Summary( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many parameters here have same type, wanted to make the data flows more explicit
| totalProblemCount = totalProblemCount, | ||
| uniqueProblemCount = problemCauses.size, | ||
| deferredProblemCount = deferredProblemCount, | ||
| consoleProblemCount = totalProblemCount - suppressedSilentlyProblemCount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an actual change
| uniqueProblemCount = problemCauses.size, | ||
| deferredProblemCount = deferredProblemCount, | ||
| consoleProblemCount = totalProblemCount - suppressedSilentlyProblemCount, | ||
| consoleProblemCauses = ImmutableMap.copyOf(problemCauses.filterValues { it != ProblemSeverity.SuppressedSilently }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this. Since we only care for problem causes we can show on the console, decided to filter it here, and reflect that in the property name
| overflowed = true | ||
| return false | ||
| } | ||
| if (severity != ProblemSeverity.SuppressedSilently) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, we would not apply deduplication to problems with this severity, now we do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Do we cover this by integTest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do with a unit test:
Line 124 in 1ac66b7
| assertThat(accepted, equalTo(unique)) |
Also, integration tests had to be updated to reflect this, including a smoke test.
| fun onIncompatibleFeature() { | ||
| fun onIncompatibleFeature(problem: PropertyProblem) { | ||
| lock.withLock { | ||
| onProblem(problem, ProblemSeverity.SuppressedSilently) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For incompatible features, we were adding a problem to the report without letting the summarizer know - now we do report the problem so it is reflected on totalProblemCount and suppressedSilentlyProblemCount.
| * Returns whether problem was accepted. | ||
| */ | ||
| private | ||
| fun recordProblemCause(problem: PropertyProblem, severity: ProblemSeverity): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, this may return false even if the new problem upgraded the severity of a previous one
be5eed3 to
fad4665
Compare
|
|
||
| assertThat( | ||
| "HTML report JS model has wrong number of total problem(s)", | ||
| numberOfProblems(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was relying on number of problems in the data - now that we deduplicate, we need to query the totalProblemCount slot instead
| } | ||
|
|
||
| private void assertProblemsHtmlReport(HasConfigurationCacheProblemsSpec spec) { | ||
| def totalProblemCount = spec.totalProblemsCount ?: spec.uniqueProblems.size() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this meant that if totalProblemsCount == 0 we would ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Not sure, why?
| ) | ||
|
|
||
| if (spec.checkReportProblems) { | ||
| def problemMessages = problemMessages().unique() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to remove duplicates here
This comment has been minimized.
This comment has been minimized.
|
|
||
| int getProblemsWithStackTrace() { | ||
| return problems.inject(0) { a, b -> a + (b.hasStackTrace ? b.count : 0) } | ||
| return problems.inject(0) { a, b -> a + (b.hasStackTrace ? 1 : 0) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This count is unique now
fad4665 to
997b249
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The following builds have failed: |
| "Project ':' cannot access 'Project.plugins' functionality on subprojects via 'allprojects'", | ||
| "Project ':' cannot access 'Project.extensions' functionality on subprojects via 'allprojects'", | ||
| ) | ||
| // maximum number of problems we collect (should be 86520) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was what made us notice the issue with lack of deduplication. I think 86520 was what I saw with a previous version of the init script used by this test which would fail to schedule some tasks. When that was addressed, and more tasks scheduled grew, so did the number of violations.
|
@bot-gradle test RFN |
This comment has been minimized.
This comment has been minimized.
|
The following builds have passed: |
ad6ef3d to
1ac66b7
Compare
| totalProblemsCount = 2 | ||
| // gracefully degraded task appears in the report | ||
| withProblem(INVOCATION_OF_TASK_PROJECT_AT_EXECUTION_TIME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Why don't we check total problems count anymore?
🤔 I wonder, why do we need to add the same withProblem(xxx)? The deal is withProblems adds a problem to uniqueProblems set, so I'd assume that it should be unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do (it is always checked), but since it is the same as the number of unique problems, it is not necessary to explicitly state it.
The set of expected unique problems is actually a list. Each expected unique problem must match each actual unique problem message, in the same position they are expected to appear. The matching is based on message alone, not location, but uniqueness considers both message and location, so the same kind of problem (with identical messages) may appear multiple times.
I think Mike's PR will allow us to match locations as well.
| * Builds the console feedback string for configuration cache problems. | ||
| * | ||
| * Rules: | ||
| * - If there are reportable problems, print a summary header and a curated list of up to [MAX_CONSOLE_PROBLEMS]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Should we use "console" instead of "reportable" ?
| if (reportableProblemCauseCount > MAX_CONSOLE_PROBLEMS) { | ||
| appendLine("plus ${reportableProblemCauseCount - MAX_CONSOLE_PROBLEMS} more problems. Please see the report for details.") | ||
| if (uniqueProblemCount > MAX_CONSOLE_PROBLEMS) { | ||
| appendLine("plus ${uniqueProblemCount - MAX_CONSOLE_PROBLEMS} more problems. Please see the report for details.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Did we change the logic here?
Before, we were checking reportableProblemCauseCount > MAX_CONSOLE_PROBLEMS, and then print "plus ${reportableProblemCauseCount - MAX_CONSOLE_PROBLEMS} more problems."
Now we do same, but instead of reportableProblems we use uniqueProblems, which includes suppressed silently problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks! I will add a test and fix this.
Issue: #36046 (see reproducer there)
Without the fix:
Report size: 107M
With the fix:
Report size: 153K
Reviewing cheatsheet
Before merging the PR, comments starting with