Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

Fix issue where doublestar globbing only worked at a single level #268

Merged
merged 2 commits into from
Sep 17, 2021

Conversation

djaglowski
Copy link
Member

See opentelemetry-collector-contrib#4628

The PR also refactors file finding into a more testable structure, and adds tests.

@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #268 (2c3defd) into main (bef2800) will increase coverage by 0.0%.
The diff coverage is 85.7%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #268   +/-   ##
=====================================
  Coverage   76.4%   76.4%           
=====================================
  Files         94      95    +1     
  Lines       4453    4454    +1     
=====================================
+ Hits        3403    3404    +1     
  Misses       719     719           
  Partials     331     331           
Impacted Files Coverage Δ
operator/builtin/input/file/finder.go 77.7% <77.7%> (ø)
operator/builtin/input/file/config.go 77.0% <100.0%> (-0.4%) ⬇️
operator/builtin/input/file/file.go 72.3% <100.0%> (+<0.1%) ⬆️

@djaglowski
Copy link
Member Author

The fix boiled down to switching from filepath.Glob to doublestar.Glob, within file_input's poll loop. Because of this, performance is a concern. However, benchmarks appear to show a small improvement:

This branch:

> make bench
go test -benchmem -run=^$ -bench ^* ./...
...
goos: darwin
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/input/file
cpu: Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
BenchmarkFileInput/Single-8               468024              2194 ns/op             640 B/op          9 allocs/op
BenchmarkFileInput/Glob-8                 118584             10311 ns/op            2570 B/op         36 allocs/op
BenchmarkFileInput/MultiGlob-8            118840             11821 ns/op            2572 B/op         36 allocs/op
BenchmarkFileInput/MaxConcurrent-8         80212             12729 ns/op            2564 B/op         36 allocs/op
BenchmarkFileInput/FngrPrntLarge-8        472976              2140 ns/op             641 B/op          9 allocs/op
BenchmarkFileInput/FngrPrntSmall-8        641103              1933 ns/op             640 B/op          9 allocs/op

main:

> make bench
go test -benchmem -run=^$ -bench ^* ./...
...
goos: darwin
goarch: amd64
pkg: github.com/open-telemetry/opentelemetry-log-collection/operator/builtin/input/file
cpu: Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
BenchmarkFileInput/Single-8               451371              2344 ns/op             640 B/op          9 allocs/op
BenchmarkFileInput/Glob-8                 114730             12301 ns/op            2573 B/op         36 allocs/op
BenchmarkFileInput/MultiGlob-8            115032             12374 ns/op            2573 B/op         36 allocs/op
BenchmarkFileInput/MaxConcurrent-8         80317             12709 ns/op            2563 B/op         36 allocs/op
BenchmarkFileInput/FngrPrntLarge-8        629154              1916 ns/op             641 B/op          9 allocs/op
BenchmarkFileInput/FngrPrntSmall-8        656941              2148 ns/op             640 B/op          9 allocs/op

@djaglowski djaglowski marked this pull request as ready for review September 16, 2021 20:18
@djaglowski djaglowski requested review from a team and jsirianni September 16, 2021 20:18
@djaglowski djaglowski merged commit 48fd163 into open-telemetry:main Sep 17, 2021
@djaglowski djaglowski deleted the finder branch September 17, 2021 14:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants