fix: improve FileFilter metrics

[octavian-snyk]

Description

This PR improves FileFilter analytics by replacing per-run metric scopes with stable keys that aggregate repeated runs. Runs are grouped by the feature-flag combination they use, allowing input/output file counts, rule counts, and durations, to be compared across variants. Temporary legacy variants: var0 (no feature flags), var1 (metaCharFix), var2 (gitIgnoreRespectTrackedFiles).

This PR also introduces a concurrency-safe metrics accumulator.

Tests cover every feature-flag variant, repeated and concurrent runs, nil recorders, aggregation behavior, and FileFilter metrics reaching invocation analytics.

Checklist

• Tests added and all succeed (make test)
• Regenerated mocks, etc. (make generate)
• Linted (make lint)
• Test your changes work for the CLI
  1. Clone / pull the latest CLI main.
  2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
     • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
  3. Run go mod tidy in the cliv2 directory.
  4. Run the CLI tests and do any required manual testing.
  5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
  • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

---

Note

Medium Risk
Analytics key schema and aggregation semantics change for FileFilter metrics consumed via invocation analytics; filtering behavior is unchanged when metrics are disabled.

Overview
FileFilter analytics no longer use per-run scope IDs (file-filter.<scopeID>.<metric>). Metrics are written under stable keys grouped by feature-flag combination (var0 / var1 / var2, or file-filter.<metric> when both flags are on), and repeated runs aggregate into the same keys instead of overwriting each other.

A new metrics.Accumulator backs this: it sums integer metrics (input/output counts, durations), keeps per-key maximums, and records bools without aggregation, with mutex-protected global state shared across accumulators on different recorders. ResetAccumulated clears that state for tests. FileFilter wires WithMetrics through an accumulator and skips flag reads when nothing is recording.

Metric names and semantics shift: e.g. survivingFileCount → filter.outputFileCount, rulesBuildDurationMs → rules.durationMs, plus filter.inputFileCount; duration fields are summed across calls for a variant. Feature flags are recorded as feature.metaCharFix and feature.includeTracked. RecorderFake gains NewRecorderFake() so maps are always initialized.

Tests cover aggregation, concurrency, variants, and invocation analytics extension keys; workflow tests expect the new file-filter.var0.* shape.

^{Reviewed by Cursor Bugbot for commit 8b8d122. Bugbot is set up for automated code reviews on this repo. Configure here.}

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues
✅	Secrets	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[octavian-snyk]

snyk-pr-review-bot and cursor's points were already taken into consideration as tradeoffs for this implementation.
Accumulator instances cannot be simply scoped to their Analytics object counterpart easily, so the bleeding behaviour was chosen.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit e44fac5. Configure here.}

[danskmt]

Approved, left some comments.
Please remove some of the comments as they are not really needed

[PeterSchafer]

Suggestion: Ignoring for now that this is called Accumulator, You could actually track all the entries using map[string][]int and calculate different things from it, sum, mean, median ...

[PeterSchafer]

This is more an optional suggestion. or design feedback

[PeterSchafer]

Question: what do we do when the feature flags are being removed and the feature will just be enabled?

[PeterSchafer]

Should var0 just be something like default file filter configuration?

[octavian-snyk]

I'm not sure we have the character allowance for a long descriptive name. I went with var0, var1, etc., in the first place because names like "legacy" or "metaFix" were too long.
What if we drop the var3 prefix entirely instead? Then var0, var1, and var2 can act as our temporary ones.

file-filter.filter.inputFileCount instead of file-filter.var3.filter.inputFileCount
file-filter.var0.filter.inputFileCount would remain file-filter.var0.filter.inputFileCount

[PeterSchafer]

sounds good, default is no var

[danskmt]

they all have same prefix name metricFileFilter, but their values are different. I would suggest improving this naming. Since we are inside file_filter.go, we could use metricVariantXYZ for the var ones, metricFilterXYZ for the 'filter.' ones and metricFeatureXYZ for the 'feature.' ones, WDYT?

[danskmt]

same here, and organize them better so the similar ones stays together

[danskmt]

LGTM - Added some suggestions

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Architecture Violation 🔴 [critical] The accumulated map is defined as a package-level global variable. This violates the 'Multi-consumer fitness' rule in AGENTS.md which forbids package-level mutable state in favor of per-invocation state. In long-running processes like IDEs or servers, metrics from independent scan requests will be aggregated together. This results in incorrect reporting (e.g., Scan B reporting the sum of Scan A and Scan B) and potential data leakage between logical sessions. The state should be scoped to the InvocationContext or the Accumulator instance itself. var ( accumulatedMu sync.Mutex accumulated = map[string]int{} ) Incorrect Aggregation 🟠 [major] The PR changes duration metrics (metricFilterDurationMs, metricRulesBuildDurationMs) to use recordSumLazy, which calls Accumulator.AddToSum. Because the accumulator aggregates process-wide, these metrics will now report the cumulative duration of all scans performed since the process started, rather than the duration of the current scan. This makes performance tracking impossible for consumers like the Snyk CLI or IDEs once multiple runs occur. defer fw.recordSumLazy(variant, metricRulesBuildDurationMs, func() int { return int(time.Since(start).Milliseconds()) }) Potential Panic 🟡 [minor] The PR removes the lazy initialization checks (e.g., if r.IntValues == nil) from the RecorderFake methods. While a constructor NewRecorderFake was added, any existing or future code that instantiates the struct manually (e.g., &RecorderFake{}) will now panic on the first write to a map field. Maintaining defensive initialization within the methods is safer for a shared testing utility. r.IntValues[key] = value }

📚 Repository Context Analyzed This review considered 22 relevant code sections from 15 files (average relevance: 0.92) internal/metrics/metrics.go (lines 1-9) pkg/mocks/workflow.go (lines 232-423) pkg/apiclients/mocks/testapi.go (lines 1-225) pkg/configuration/constants.go (lines 3-96) pkg/apiclients/fileupload/batch.go (lines 1-208) pkg/mocks/config_storage.go (lines 1-94) pkg/local_workflows/ignore_workflow/ignore_workflow.go (lines 1-98) ... and 8 more file(s)

🤖 Repository instructions applied (from AGENTS.md)

[CatalinSnyk]

Also looks good to me, just wondering if the PR bot feedback was address/dismissed before 😄

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Global State Risk 🔴 [critical] The accumulated map is defined as a package-level variable. In multi-consumer environments like IDEs or MCP servers, multiple logically separate operations (e.g., scanning different projects or for different organizations) may run in the same process. Because this state is shared globally, metrics from one operation will leak into the aggregates of another, leading to incorrect telemetry data. State should be managed per-Accumulator instance or via a context-bound provider rather than package-level variables. var ( accumulatedMu sync.Mutex accumulated = map[string]int{} ) Breaking Test API 🟠 [major] Removing the lazy map initialization in AddExtensionIntegerValue and related methods makes RecorderFake unsafe to use as a zero-value (e.g., &RecorderFake{}). Any existing downstream test that instantiates this struct directly rather than through the new NewRecorderFake helper will now trigger a nil pointer panic when metrics are recorded. As this is a public test utility, the lazy initialization should be retained to maintain backward compatibility. func (r *RecorderFake) AddExtensionIntegerValue(key string, value int) { r.mu.Lock() defer r.mu.Unlock() r.IntValues[key] = value } func (r *RecorderFake) AddExtensionStringValue(key string, value string) { r.mu.Lock() defer r.mu.Unlock() r.StringValues[key] = value } func (r *RecorderFake) AddExtensionBoolValue(key string, value bool) { r.mu.Lock() defer r.mu.Unlock() r.BoolValues[key] = value } Breaking Telemetry Schema 🟠 [major] The PR removes the per-run scopeID from metric keys and changes metric names (e.g., survivingFileCount to filter.outputFileCount). This is a breaking change for downstream analytics consumers, dashboards, and alerting systems that rely on the previous key format file-filter.<scopeID>.<metric>. According to AGENTS.md, 'Stability is a feature' and changing exported signatures or identifiers is breaking. const ( metricPrefix = "file-filter" // prefix for all file-filter analytics keys metricVariantLegacy = "var0" // neither feature flag enabled metriVariantMetacharFix = "var1" // FF_FILE_FILTER_METACHARACTER_FIX only metricVariantTrackedFiles = "var2" // FF_GITIGNORE_RESPECT_TRACKED_FILES only metricVariantBothFixes = "" // both feature flags enabled; the variant segment is omitted metricFilterInputFileCount = "filter.inputFileCount" // sum, across all GetFilteredFiles calls for the variant, of files offered before exclusion metricFilterOutputFileCount = "filter.outputFileCount" // sum, across all GetFilteredFiles calls for the variant, of files that passed exclusion metricFilterDurationMs = "filter.durationMs" // sum, across all GetFilteredFiles calls for the variant, of elapsed time including the caller's drain of the result channel metricRulesBuildDurationMs = "rules.durationMs" // sum, across all GetRules calls for the variant, of elapsed time for directory walk, ignore discovery, and buildGlobs // Record feature flags alongside the variant so consumers need not decode its name. metricFeatureMetacharFix = "feature.metaCharFix" // whether FF_FILE_FILTER_METACHARACTER_FIX applied to the run metricFeatureTrackedFiles = "feature.includeTracked" // whether FF_GITIGNORE_RESPECT_TRACKED_FILES applied to the run )

📚 Repository Context Analyzed This review considered 23 relevant code sections from 15 files (average relevance: 0.92) internal/metrics/metrics.go (lines 1-9) pkg/apiclients/fileupload/batch.go (lines 1-208) pkg/mocks/workflow.go (lines 232-423) pkg/apiclients/mocks/testapi.go (lines 1-225) pkg/analytics/instrumentation_collector.go (lines 1-249) pkg/configuration/constants.go (lines 3-96) pkg/mocks/config_storage.go (lines 1-94) ... and 8 more file(s)

🤖 Repository instructions applied (from AGENTS.md)