RecipeRunStats several times counts the same recipe runs with different params

[arodionov]

When the same recipe is executed several times with different parameters, the tag("name", recipe.getName()) will be the same, and it will produce wrong (n-times bigger) stats for source-files ("The number of source files the recipe ran over") in RecipeRunStats.

As a fix, a HashSet was introduced to track all visited sources.

[arodionov]

Checked a new version on the same repo #4876 (comment)
The org.openrewrite.staticanalysis.CommonStaticAnalysis and org.openrewrite.java.migrate.UpgradeToJava17 have the same numbers of sourceFiles (12056) in a RecipeRunStats table.

[jkschneider]

I'm not clear on whether this table is shared across repositories and whether then it will undercount source file paths that reoccur across repos.

[arodionov]

@jkschneider I've tested the mod-cli with the fix on the Apache organization. The new version gives correct and consistent results for org.openrewrite.staticanalysis.CommonStaticAnalysis and org.openrewrite.java.migrate.UpgradeToJava17

[okundzich]

@arodionov would you check on what Jonathan asked about -

whether this table is shared across repositories and whether then it will undercount source file paths that reoccur across repos

[arodionov]

@okundzich I thought that running recipes across an organization with multiple repositories should check this.
But as I understand, checking this via mod-cli is not enough, and it should be checked in a Saas platform.

[okundzich]

please make sure you have same file names across repos while testing

[arodionov]

After the source code analysis for mod-cli and moderne-worker can conclude that RecipeRunStats (which contains the initial stats) is not shared across repositories.

For mod-cli, the RecipeRunStats initialised per sourceSet, per recipe as
new RecipeScheduler().scheduleRun(recipe, lss, ctx, 1, 1)

For moderne-worker, the RecipeRunStats initialised as a part of new CheckpointingRecipeRun.StandardDataTables(), during the new StoredRecipeRun(...) constructor call:
https://github.com/moderneinc/moderne-worker/blob/550b08b827213533b710d546628405e8bba5f523/src/main/java/io/moderne/worker/execution/StoredRecipeRun.java#L590-L604
The new StoredRecipeRun(...) is also calling per repository

[timtebeek]

Thanks for confirming, and the helpful link into moderne-worker. With that I'm leaning towards a merge: we're copying a pattern we already used for sourceFileChanged to add a sourceFileVisited, with both the CLI and worker only using those per repository from what we've seen here. Nice to see this fixed.

We can of course wait a day or two before we merge in case Jonathan or Olga has any concerns (there's a US public holiday), as there's no rush getting this out.