Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: promtail race fixes #12656

Merged
merged 8 commits into from
Apr 19, 2024
Merged

fix: promtail race fixes #12656

merged 8 commits into from
Apr 19, 2024

Conversation

paul1r
Copy link
Collaborator

@paul1r paul1r commented Apr 17, 2024

What this PR does / why we need it:
Multiple data race fixes for promtail code

Which issue(s) this PR fixes:
Relates to #8586

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@paul1r paul1r requested a review from a team as a code owner April 17, 2024 14:25
@paul1r paul1r changed the title bug: promtail race fixes fix: promtail race fixes Apr 17, 2024
@paul1r
Copy link
Collaborator Author

paul1r commented Apr 17, 2024

Before the changes, the Go data race detector found many issues.

After these changes, test results were clean:

go test -count=1 ./...
?   	github.com/grafana/loki/v3/clients/pkg/promtail/api	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/client/fake	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/limit	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/server	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/server/ui	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/journal	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/serverutils	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/target	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/testutils	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/windows	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/windows/win_eventlog	[no test files]
ok  	github.com/grafana/loki/v3/clients/pkg/promtail	21.919s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/client	7.217s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/config	2.298s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/discovery/consulagent	3.287s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/positions	1.421s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/scrapeconfig	3.001s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/azureeventhubs	2.294s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/cloudflare	3.283s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/docker	7.410s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/file	49.711s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/gcplog	4.311s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/gelf	3.331s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/heroku	5.683s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/kafka	4.752s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/lokipush	3.658s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/stdin	1.718s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/syslog	4.350s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/syslog/syslogparser	1.322s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/utils	5.693s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/wal	22.589s
progers@Pauls-MacBook-Pro promtail % go test -count=1 -race ./...
?   	github.com/grafana/loki/v3/clients/pkg/promtail/api	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/client/fake	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/limit	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/server	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/server/ui	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/journal	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/serverutils	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/target	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/testutils	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/windows	[no test files]
?   	github.com/grafana/loki/v3/clients/pkg/promtail/targets/windows/win_eventlog	[no test files]
ok  	github.com/grafana/loki/v3/clients/pkg/promtail	23.632s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/client	8.423s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/config	3.990s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/discovery/consulagent	3.646s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/positions	2.896s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/scrapeconfig	4.948s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/azureeventhubs	6.002s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/cloudflare	3.686s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/docker	9.547s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/file	53.261s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/gcplog	5.865s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/gelf	5.049s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/heroku	7.793s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/kafka	6.362s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/lokipush	5.958s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/stdin	3.850s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/syslog	7.263s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/targets/syslog/syslogparser	3.561s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/utils	8.077s
ok  	github.com/grafana/loki/v3/clients/pkg/promtail/wal	24.555s

Comment on lines +63 to +64
// Create a channel for log messages
logCh := make(chan string, 100) // Buffered channel to avoid blocking
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was the logging change required for fixing a test race here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a race related to the t essentially. The t.Logf("received request: %s", counts) was in a goroutine and was fighting with t.Logf("started test server at URL %s", testServer.URL)

clients/pkg/promtail/targets/file/filetarget.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome @paul1r thanks!

@cyriltovena cyriltovena merged commit 4e04d07 into main Apr 19, 2024
14 checks passed
@cyriltovena cyriltovena deleted the paul1r/promtail_race_fixes branch April 19, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants