Skip to content

RecipeRunStats several times counts the same recipe runs with different params #5408

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.

Sign up for GitHub

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

Merged
timtebeek merged 3 commits into from
May 27, 2025
Merged

RecipeRunStats several times counts the same recipe runs with different params #5408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8323be
- reproducer RecipeRunStatsTest with non-correct source files count
arodionov May 7, 2025
65f7c84
- fix count by adding a HashSet with visited sources
Laurens-W May 9, 2025
43b40a9
- moved recordSourceVisited to the upper level to execute once for th…
Laurens-W May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 52 additions & 50 deletions rewrite-core/src/main/java/org/openrewrite/scheduling/RecipeRunCycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,65 +174,67 @@ public LSS editSources(LSS sourceSet) {
// skip edits made to generated source files so that they don't show up in a diff
// that later fails to apply on a freshly cloned repository
// consider any recipes adding new messages as a changing recipe (which can request another cycle)
return sourceSetEditor.apply(sourceSet, sourceFile ->
allRecipeStack.reduce(sourceSet, recipe, ctx, (source, recipeStack) -> {
Recipe recipe = recipeStack.peek();
if (source == null) {
return null;
}

SourceFile after = source;

try {
Duration duration = Duration.ofNanos(System.nanoTime() - cycleStartTime);
if (duration.compareTo(ctx.getMessage(ExecutionContext.RUN_TIMEOUT, Duration.ofMinutes(4))) > 0) {
if (thrownErrorOnTimeout.compareAndSet(false, true)) {
RecipeTimeoutException t = new RecipeTimeoutException(recipe);
ctx.getOnError().accept(t);
ctx.getOnTimeout().accept(t, ctx);
}
return source;
return sourceSetEditor.apply(sourceSet, sourceFile -> {
recipeRunStats.recordSourceVisited(sourceFile);
return allRecipeStack.reduce(sourceSet, recipe, ctx, (source, recipeStack) -> {
Recipe recipe = recipeStack.peek();
if (source == null) {
return null;
}

if (ctx.getMessage(PANIC) != null) {
return source;
}
SourceFile after = source;

TreeVisitor<?, ExecutionContext> visitor = recipe.getVisitor();
// set root cursor as it is required by the `ScanningRecipe#isAcceptable()`
visitor.setCursor(rootCursor);
try {
Duration duration = Duration.ofNanos(System.nanoTime() - cycleStartTime);
if (duration.compareTo(ctx.getMessage(ExecutionContext.RUN_TIMEOUT, Duration.ofMinutes(4))) > 0) {
if (thrownErrorOnTimeout.compareAndSet(false, true)) {
RecipeTimeoutException t = new RecipeTimeoutException(recipe);
ctx.getOnError().accept(t);
ctx.getOnTimeout().accept(t, ctx);
}
return source;
}

after = recipeRunStats.recordEdit(recipe, () -> {
if (visitor.isAcceptable(source, ctx)) {
// propagate shared root cursor
//noinspection DataFlowIssue
return (SourceFile) visitor.visit(source, ctx, rootCursor);
if (ctx.getMessage(PANIC) != null) {
return source;
}
return source;
});

if (after != source) {
madeChangesInThisCycle.add(recipe);
recordSourceFileResult(source, after, recipeStack, ctx);
if (source.getMarkers().findFirst(Generated.class).isPresent()) {
// skip edits made to generated source files so that they don't show up in a diff
// that later fails to apply on a freshly cloned repository
TreeVisitor<?, ExecutionContext> visitor = recipe.getVisitor();
// set root cursor as it is required by the `ScanningRecipe#isAcceptable()`
visitor.setCursor(rootCursor);

after = recipeRunStats.recordEdit(recipe, () -> {
if (visitor.isAcceptable(source, ctx)) {
// propagate shared root cursor
//noinspection DataFlowIssue
return (SourceFile) visitor.visit(source, ctx, rootCursor);
}
return source;
});

if (after != source) {
madeChangesInThisCycle.add(recipe);
recordSourceFileResult(source, after, recipeStack, ctx);
if (source.getMarkers().findFirst(Generated.class).isPresent()) {
// skip edits made to generated source files so that they don't show up in a diff
// that later fails to apply on a freshly cloned repository
return source;
}
recipeRunStats.recordSourceFileChanged(source, after);
} else if (ctx.hasNewMessages()) {
// consider any recipes adding new messages as a changing recipe (which can request another cycle)
madeChangesInThisCycle.add(recipe);
ctx.resetHasNewMessages();
}
recipeRunStats.recordSourceFileChanged(source, after);
} else if (ctx.hasNewMessages()) {
// consider any recipes adding new messages as a changing recipe (which can request another cycle)
madeChangesInThisCycle.add(recipe);
ctx.resetHasNewMessages();
} catch (Throwable t) {
after = handleError(recipe, source, after, t);
}
} catch (Throwable t) {
after = handleError(recipe, source, after, t);
}
if (after != null && after != source) {
after = addRecipesThatMadeChanges(recipeStack, after);
}
return after;
}, sourceFile)
if (after != null && after != source) {
after = addRecipesThatMadeChanges(recipeStack, after);
}
return after;
}, sourceFile);
}
);
}

Expand Down
9 changes: 8 additions & 1 deletion rewrite-core/src/main/java/org/openrewrite/table/RecipeRunStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

public class RecipeRunStats extends DataTable<RecipeRunStats.Row> {
private final MeterRegistry registry = new SimpleMeterRegistry();
private final Set<Path> sourceFileVisited = new HashSet<>();
private final Set<Path> sourceFileChanged = new HashSet<>();

public RecipeRunStats(Recipe recipe) {
Expand All @@ -43,6 +44,12 @@ public RecipeRunStats(Recipe recipe) {
"Statistics used in analyzing the performance of recipes.");
}

public void recordSourceVisited(@Nullable SourceFile source) {
if (source != null) {
sourceFileVisited.add(source.getSourcePath());
}
}

public void recordSourceFileChanged(@Nullable SourceFile before, @Nullable SourceFile after) {
if (after != null) {
sourceFileChanged.add(after.getSourcePath());
Expand Down Expand Up @@ -73,7 +80,7 @@ public void flush(ExecutionContext ctx) {
Timer scanner = registry.find("rewrite.recipe.scan").tag("name", recipeName).timer();
Row row = new Row(
recipeName,
Long.valueOf(editor.count()).intValue(),
sourceFileVisited.size(),
sourceFileChanged.size(),
scanner == null ? 0 : (long) scanner.totalTime(TimeUnit.NANOSECONDS),
scanner == null ? 0 : scanner.takeSnapshot().percentileValues()[0].value(TimeUnit.NANOSECONDS),
Expand Down
104 changes: 66 additions & 38 deletions rewrite-core/src/test/java/org/openrewrite/table/RecipeRunStatsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,68 +19,66 @@
import org.junit.jupiter.api.Test;
import org.openrewrite.*;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.text.PlainText;
import org.openrewrite.text.PlainTextVisitor;

import static org.assertj.core.api.Assertions.assertThat;
import static org.openrewrite.test.SourceSpecs.text;

class RecipeRunStatsTest implements RewriteTest {
@AllArgsConstructor
class RecipeWithApplicabilityTest extends Recipe {
@Override
public String getDisplayName() {
return "Recipe with an applicability test";
}

@AllArgsConstructor
static class RecipeWithApplicabilityTest extends Recipe {
@Override
public String getDisplayName() {
return "Recipe with an applicability test";
}
@Override
public String getDescription() {
return "This recipe is a test utility which exists to exercise RecipeRunStats.";
}

@Override
public String getDescription() {
return "This recipe is a test utility which exists to exercise RecipeRunStats.";
}
@Option(displayName = "New text", example = "txt")
String newText;

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(
new PlainTextVisitor<>() {
@Override
public PlainText visitText(PlainText text, ExecutionContext ctx) {
if (!"sam".equals(text.getText())) {
return SearchResult.found(text);
}
return text;
}
},
new PlainTextVisitor<>() {
@Override
public PlainText visitText(PlainText tree, ExecutionContext ctx) {
return tree.withText("sam");
@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(
new PlainTextVisitor<>() {
@Override
public PlainText visitText(PlainText text, ExecutionContext ctx) {
if (!"sam".equals(text.getText())) {
return SearchResult.found(text);
}
});
}
return text;
}
},
new PlainTextVisitor<>() {
@Override
public PlainText visitText(PlainText tree, ExecutionContext ctx) {
return tree.withText(newText);
}
}
);
}
}

@Override
public void defaults(RecipeSpec spec) {
spec.recipe(new RecipeWithApplicabilityTest());
}
class RecipeRunStatsTest implements RewriteTest {

@DocumentExample
@Test
void singleRow() {
rewriteRun(
spec -> spec.dataTable(RecipeRunStats.Row.class, rows -> {
spec -> spec
.recipe(new RecipeWithApplicabilityTest("sam"))
.dataTable(RecipeRunStats.Row.class, rows -> {
assertThat(rows)
.as("Running a single recipe on a single source should produce a single row in the RecipeRunStats table")
.hasSize(1);
RecipeRunStats.Row row = rows.get(0);
assertThat(row.getRecipe()).endsWith("RecipeWithApplicabilityTest");
assertThat(row.getSourceFiles())
.as("Test framework will invoke the recipe once when it is expected to make a change, " +
"then once again when it is expected to make no change")
.isEqualTo(2);
.isEqualTo(1);
assertThat(row.getEditMaxNs()).isGreaterThan(0);
assertThat(row.getEditTotalTimeNs())
.as("Cumulative time should be greater than any single visit time")
Expand All @@ -89,4 +87,34 @@ void singleRow() {
text("samuel", "sam")
);
}

@Test
void sourceFilesCountStatsForSameRecipe() {
rewriteRun(
spec -> spec
.recipeFromYaml("""
---
type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.SeveralMethodNameChangeRecipes
description: Test.
recipeList:
- org.openrewrite.table.RecipeWithApplicabilityTest:
newText: sam1
- org.openrewrite.table.RecipeWithApplicabilityTest:
newText: sam2
""",
"org.openrewrite.SeveralMethodNameChangeRecipes")
.dataTable(RecipeRunStats.Row.class, rows -> {
assertThat(rows)
.as("Running declarative recipe with parametrized recipe a single source should produce a two rows in the RecipeRunStats table")
.hasSize(2);
for (RecipeRunStats.Row row : rows) {
assertThat(row.getSourceFiles())
.as("If the same recipe runs with different parameters it shouldn't increment several times source files count")
.isEqualTo(1);
}
}).expectedCyclesThatMakeChanges(2),
text("sem", "sam2")
);
}
}