Skip to content

Conversation

rajagopalanand
Copy link
Contributor

@rajagopalanand rajagopalanand commented Apr 11, 2025

The count() acquires a lock but it is not necessary. The method makes a copy of store alerts by calling a.alerts.List(). The a.alerts.List() method acquires its own internal lock
Similarly, a.marker.Status(..) also has its own lock. Therefore, removing the lock from count() should be safe.

count() method is called during scrapes and since it acquires a lock, it can cause bottlenecks during scraping

@rajagopalanand rajagopalanand marked this pull request as ready for review April 11, 2025 17:00
@grobinson-grafana
Copy link
Collaborator

This seems reasonable to me. Have you tested it with the race detector just out of interest?

@rajagopalanand
Copy link
Contributor Author

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

go test -race  `go list ./... | grep -v notify/email` -count=1                                    
?   	github.com/prometheus/alertmanager/api	[no test files]
?   	github.com/prometheus/alertmanager/api/metrics	[no test files]
ok  	github.com/prometheus/alertmanager/api/v2	1.082s
?   	github.com/prometheus/alertmanager/api/v2/client	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/alert	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/alertgroup	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/general	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/receiver	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/silence	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/models	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/alert	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/alertgroup	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/general	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/receiver	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/silence	[no test files]
?   	github.com/prometheus/alertmanager/asset	[no test files]
ok  	github.com/prometheus/alertmanager/cli	1.121s
ok  	github.com/prometheus/alertmanager/cli/config	1.050s
?   	github.com/prometheus/alertmanager/cli/format	[no test files]
ok  	github.com/prometheus/alertmanager/cluster	3.318s
?   	github.com/prometheus/alertmanager/cluster/clusterpb	[no test files]
ok  	github.com/prometheus/alertmanager/cmd/alertmanager	1.145s
?   	github.com/prometheus/alertmanager/cmd/amtool	[no test files]
ok  	github.com/prometheus/alertmanager/config	1.231s
ok  	github.com/prometheus/alertmanager/config/receiver	1.085s
ok  	github.com/prometheus/alertmanager/dispatch	5.321s
?   	github.com/prometheus/alertmanager/examples/webhook	[no test files]
ok  	github.com/prometheus/alertmanager/featurecontrol	1.027s
ok  	github.com/prometheus/alertmanager/inhibit	1.041s
ok  	github.com/prometheus/alertmanager/matcher/compat	1.018s
ok  	github.com/prometheus/alertmanager/matcher/compliance	1.021s
ok  	github.com/prometheus/alertmanager/matcher/parse	1.029s
ok  	github.com/prometheus/alertmanager/nflog	2.033s
ok  	github.com/prometheus/alertmanager/nflog/nflogpb	1.012s
ok  	github.com/prometheus/alertmanager/notify	1.658s
ok  	github.com/prometheus/alertmanager/notify/discord	1.229s
ok  	github.com/prometheus/alertmanager/notify/jira	1.422s
ok  	github.com/prometheus/alertmanager/notify/msteams	1.222s
ok  	github.com/prometheus/alertmanager/notify/msteamsv2	1.209s
ok  	github.com/prometheus/alertmanager/notify/opsgenie	1.217s
ok  	github.com/prometheus/alertmanager/notify/pagerduty	1.266s
ok  	github.com/prometheus/alertmanager/notify/pushover	1.096s
ok  	github.com/prometheus/alertmanager/notify/rocketchat	1.065s
ok  	github.com/prometheus/alertmanager/notify/slack	1.183s
ok  	github.com/prometheus/alertmanager/notify/sns	1.158s
ok  	github.com/prometheus/alertmanager/notify/telegram	1.118s
?   	github.com/prometheus/alertmanager/notify/test	[no test files]
ok  	github.com/prometheus/alertmanager/notify/victorops	1.193s
ok  	github.com/prometheus/alertmanager/notify/webex	1.114s
ok  	github.com/prometheus/alertmanager/notify/webhook	1.089s
ok  	github.com/prometheus/alertmanager/notify/wechat	1.097s
ok  	github.com/prometheus/alertmanager/pkg/labels	1.028s
?   	github.com/prometheus/alertmanager/pkg/modtimevfs	[no test files]
?   	github.com/prometheus/alertmanager/provider	[no test files]
ok  	github.com/prometheus/alertmanager/provider/mem	5.564s
ok  	github.com/prometheus/alertmanager/silence	2.052s
?   	github.com/prometheus/alertmanager/silence/silencepb	[no test files]
ok  	github.com/prometheus/alertmanager/store	1.030s
ok  	github.com/prometheus/alertmanager/template	1.123s
?   	github.com/prometheus/alertmanager/test/cli	[no test files]
ok  	github.com/prometheus/alertmanager/test/cli/acceptance	3.630s
?   	github.com/prometheus/alertmanager/test/with_api_v2	[no test files]
ok  	github.com/prometheus/alertmanager/test/with_api_v2/acceptance	28.088s
ok  	github.com/prometheus/alertmanager/timeinterval	1.025s
ok  	github.com/prometheus/alertmanager/types	1.024s
?   	github.com/prometheus/alertmanager/ui	[no test files]
?   	github.com/prometheus/alertmanager/ui/react-app	[no test files]

@grobinson-grafana
Copy link
Collaborator

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

Not the tests, but run a build Alertmanager of with this patch with the race detector enabled.

@rajagopalanand
Copy link
Contributor Author

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

Not the tests, but run a build Alertmanager of with this patch with the race detector enabled.

Will do this and report back

@rajagopalanand
Copy link
Contributor Author

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

Not the tests, but run a build Alertmanager of with this patch with the race detector enabled.

I built AM with -race and ran it for a day. Created bunch of alerts, silences, aggregation groups and did not see any logs related to data race

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants