elastic / elastic/beats

[refactor-opportunist] Consolidate filestream runtime-settings normalization across parsing layers

Open
#53,198 1 comment 0 reactions 0 assignees View on GitHub
needs_team
Dominant language
Go
Stars
12.7k
Forks
5k
Avg merge
2d 1h
Merged PRs (30d)
370

Description

## 🏗️ Refactor Proposal

**Summary:** Consolidate filestream `clean_inactive` runtime normalization into a shared helper used by both outer filestream config and `input-logfile` manager parsing, then extend the same pattern to the remaining duplicated runtime fields.

## Problem
`filestream` still has a split runtime configuration boundary where behavior is normalized in more than one layer.

Concrete code evidence:
- `filebeat/input/filestream/config.go:79` says: `"clean_inactive" is parsed, again, and used by internal/input-logfile/manager.go`.
- `filebeat/input/filestream/config.go:88-99` also documents that `take_over`, `allow_deprecated_id_duplication`, and `legacy_clean_inactive` are independently parsed by `InputManager.Create`.
- `filebeat/input/filestream/internal/input-logfile/manager.go:168` says: `All those values are duplicated from the Filestream configuration`.

This seam is active/high-churn:
- `git log --since='60 days ago' --oneline -- filebeat/input/filestream/config.go filebeat/input/filestream/input.go filebeat/input/filestream/internal/input-logfile/manager.go | wc -l` -> `8` commits.
- Recent commits include `#53136`, `#53122`, `#52797`, `#52326`, `#52147`, `#52103`, `#51278`, `#52095`.

## Proposed Approach
Create a shared runtime-settings helper in `internal/input-logfile` for behavior that must remain identical across both parsing paths, and migrate call sites incrementally.

This preserves existing behavior while removing drift-prone duplicated logic. The first proven step is consolidating `clean_inactive == 0` normalization.

## Proof of Concept
I partially implemented this refactor on one representative slice to verify viability.

**Files changed:**
- `filebeat/input/filestream/internal/input-logfile/settings.go`
- `filebeat/input/filestream/internal/input-logfile/settings_test.go`
- `filebeat/input/filestream/internal/input-logfile/manager.go`
- `filebeat/input/filestream/input.go`

**Before → After:**

`filebeat/input/filestream/input.go:153`
```go
c.CleanInactive = loginp.NormalizeCleanInactive(c.CleanInactive, c.LegacyCleanInactive)
```

`filebeat/input/filestream/internal/input-logfile/manager.go:197`
```go
settings.CleanInactive = NormalizeCleanInactive(settings.CleanInactive, settings.LegacyCleanInactive)
```

New shared helper:

`filebeat/input/filestream/internal/input-logfile/settings.go:24-31`
```go
func NormalizeCleanInactive(cleanInactive time.Duration, legacyCleanInactive bool) time.Duration {
if !legacyCleanInactive && cleanInactive == 0 {
return -1
}
return cleanInactive
}
```

Added focused coverage:

`filebeat/input/filestream/internal/input-logfile/settings_test.go:27-61`
```go
func TestNormalizeCleanInactive(t *testing.T) { ... }
```

**Verification:**
- `go test ./filebeat/input/filestream/internal/input-logfile -run TestNormalizeCleanInactive` -> `ok`
- `go test ./filebeat/input/filestream -run TestConfigValidate` -> `ok`

## Incremental Rollout Plan
This refactor can be completed incrementally:
1. Migrate shared behavior-only logic first (already proven for `clean_inactive` normalization).
2. Extract a shared `RuntimeSettings` contract in `internal/input-logfile` for other duplicated fields (`take_over`, `allow_deprecated_id_duplication`, `read_until_eof`, backoff/close interval).
3. Switch `filestream` and `InputManager.Create` to that shared contract and remove duplicated comments/logic from both sides.

## Risks and Mitigations
- Risk: config-compatibility drift while unifying parsing.
- Mitigation: keep migration behavior-preserving and add table-driven compatibility tests per field.
- Risk: widening scope too quickly across all fields.
- Mitigation: migrate one field/group at a time with targeted tests before next step.

## Evidence
- Structural duplication comments:
- `filebeat/input/filestream/config.go:79`
- `filebeat/input/filestream/config.go:88-99`
- `filebeat/input/filestream/internal/input-logfile/manager.go:168`
- Partial implementation (working slice):
- `filebeat/input/filestream/internal/input-logfile/settings.go:22-32`
- `filebeat/input/filestream/input.go:153`
- `filebeat/input/filestream/internal/input-logfile/manager.go:197`
- Duplicate check against prior opportunist findings:
- `/tmp/previous-findings.json` (no matching open issue for this filestream runtime parsing seam)

---
[What is this?](https://ela.st/github-ai-tools) | [From workflow: Refactor Opportunist](https://github.com/elastic/beats/actions/runs/34853928377)

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

Contributor guide

Open the contributing guide

Research direction

Start with filebeat/input/filestream/config.go and input.go, then compare filebeat/input/filestream/internal/input-logfile/manager.go with settings.go and settings_test.go. Run the focused normalization and ConfigValidate tests before examining the remaining duplicated runtime fields. Done means shared behavior is covered by compatibility tests and both parsing paths retain existing behavior without duplicated normalization logic.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.