promtail: Fix issue with dropping logs when a file based SD target's labels are updated #7016
Conversation
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
|
this is ready for review now |
chodges15
changed the title
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
|
Thanks for submitting a PR! I've made some suggestions but this looks reasonable.
|
Hi @MasslessParticle I don't see the suggestions you mentioned above. Is there something I need to do to see them? |
| // keep track of how many targets are using a fileEventWatcher | ||
| watcherUseCount := make(map[string]int, len(s.fileEventWatchers)) | ||
| for _, target := range s.targets { | ||
| if _, ok := watcherUseCount[target.path]; !ok { |
There was a problem hiding this comment.
we can always say watcherUserCount[target.path]++ here because 0 is the default value: https://go.dev/play/p/qotVnYhrlKK
| k := target.path | ||
| _, ok := watcherUseCount[k] |
There was a problem hiding this comment.
This can be written as
if _, ok := watcherUseCount[k]; !ok {
level.Warn(s.log).Log("msg", "failed to find file event watcher", "path", k)
continue
}to avoid the following else clause
|
Sorry about that, @afayngelerindbx! I didn't click the |
No worries. Happens to me all the time |
| @@ -373,7 +395,6 @@ func (s *targetSyncer) sync(groups []*targetgroup.Group, targetEventHandler chan | |||
| } | |||
| } | |||
| } | |||
| s.droppedTargets = dropped | |||
There was a problem hiding this comment.
moving this line doesn't seem necessary to change the behaviour of keeping or removing a file watcher, so lets not move it in this PR
| watcherUseCount := make(map[string]int, len(s.fileEventWatchers)) | ||
| for _, target := range s.targets { |
There was a problem hiding this comment.
we could simplify/remove the need for this extra loop on every sync call by storing the watcherUseCount as a field in the targetSyncer struct
…labels are updated (grafana#7016) <!-- Thanks for sending a pull request! Before submitting: 1. Read our CONTRIBUTING.md guide 2. Name your PR as `<Feature Area>: Describe your change`. a. Do not end the title with punctuation. It will be added in the changelog. b. Start with an imperative verb. Example: Fix the latency between System A and System B. c. Use sentence case, not title case. d. Use a complete phrase or sentence. The PR title will appear in a changelog, so help other people understand what your change will be. 3. Rebase your PR if it gets out of sync with main --> **What this PR does / why we need it**: Allows for modifying labels for a target and continuing to scrape logs afterwards. **Which issue(s) this PR fixes**: Fixes grafana#7001 **Special notes for your reviewer**: <!-- Note about CHANGELOG entries, if a change adds: * an important feature * fixes an issue present in a previous release, * causes a change in operation that would be useful for an operator of Loki to know then please add a CHANGELOG entry. For documentation changes, build changes, simple fixes etc please skip this step. We are attempting to curate a changelog of the most relevant and important changes to be easier to ingest by end users of Loki. Note about the upgrade guide, if this changes: * default configuration values * metric names or label names * changes existing log lines such as the metrics.go query output line * configuration parameters * anything to do with any API * any other change that would require special attention or extra steps to upgrade Please document clearly what changed AND what needs to be done in the upgrade guide. --> **Checklist** - [ ] Documentation added - [x] Tests updated - [x] Is this an important fix or new feature? Add an entry in the `CHANGELOG.md`. - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`
What this PR does / why we need it:
Allows for modifying labels for a target and continuing to scrape logs afterwards.
Which issue(s) this PR fixes:
Fixes #7001
Special notes for your reviewer:
Checklist
CHANGELOG.md.docs/sources/upgrading/_index.md