[MM-52867] feat: add advisor warn log for Elasticsearch #23729

fmartingr · 2023-06-13T13:34:50Z

Summary

This commit addresses the addition of a new warning log line for server admins that advise the usage of Elasticsearch when the post count is too big and Elastic is not enabled.

Notes

This is merely a draft, requesting some feedback since this is my first contribution to the suite:

Made a single method for the logic since the codebase is pretty big and I wasn't sure where this should land. Suggestions appreciated.
Initially I used app.GetAnalytics(), but that performs more queries that I needed and I didn't wan't to modify that method (at least not without confirming that I should). Maybe we could cache the analytics?
This will add two COUNT queries daily. It shouldn't be much in terms of performance, and since the requirements are pretty specific it shouldn't pose an issue. If you have any concerns please let me know.
I tried to replicate the same conditions used in the webapp. Please let me know if that's appropriate or if I should get the data from somewhere else.

Ticket Link

https://mattermost.atlassian.net/browse/MM-52867

Release Note

feat: advise admins of elasticsearch usage in console periodically

isacikgoz

Overall this is a great idea, I'd rather use a simple job mechanism like we do in various jobs. Eg:

mattermost/server/channels/app/server.go

Line 1220 in f7f2ebf

func runSessionCleanupJob(s *Server) {

As a future improvement, we can use the admin notices instead. WDYT?

fmartingr · 2023-06-15T08:37:24Z

Overall this is a great idea, I'd rather use a simple job mechanism like we do in various jobs. Eg: [...] As a future improvement, we can use the admin notices instead. WDYT?

Thanks for the input @isacikgoz, I will make the changes to fit the job system we have in place. I'll also take a look at notices while I'm on it.

agarciamontoro

Changes look good!

agarciamontoro · 2023-06-21T12:18:09Z

server/channels/app/server.go

+		return
+	}
+
+	if postCount > 2000000 && userCount > 500 {


Why also the user count?

This is how the webapp displays the warning as well: https://github.com/mattermost/mattermost/blob/master/webapp/channels/src/components/admin_console/workspace-optimization/dashboard_checks/performance.ts#L37

server/channels/app/server.go

isacikgoz

I think a bit of changes needed :)

server/channels/app/server.go

Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com>

fmartingr · 2023-06-21T13:04:16Z

Thanks for the reviews, addressed the above comments.

server/channels/app/server.go

agnivade

I also wanted to understand what the end goal here is. Nobody reads warning logs. It's not actionable at all.

A better way might be to have a big red banner in the system console asking admins to use elasticsearch.

agnivade · 2023-06-22T03:58:34Z

server/channels/app/server.go

+	model.CreateRecurringTaskFromNextIntervalTime("Elasticsearch workspace optimization check (startup)", func() {
+		doElasticsearchWorkspaceOptimizationCheck(s)
+	}, time.Minute*10)
+
+	// Schedule recurring job
+	model.CreateRecurringTask("Elasticsearch workspace optimization check", func() {
+		doElasticsearchWorkspaceOptimizationCheck(s)
+	}, time.Hour*24)


Are you sure this is the right thing? This sets up the same job in two different frequencies, once every 10 mins, once every 24 hours. Any reason we are doing it this way?

Probably not, I though it worked in a different way. Let me take a look.

agnivade · 2023-06-22T04:02:12Z

It shouldn't be much in terms of performance, and since the requirements are pretty specific it shouldn't pose an issue.

It does have an impact in performance. A full post count can take around 10s on large installations. That consumes a lot of DB CPU. It can be noticeable for large customers.

fmartingr · 2023-06-22T09:05:07Z

It does have an impact in performance. A full post count can take around 10s on large installations. That consumes a lot of DB CPU. It can be noticeable for large customers.

Yeah, what I meant is that this only happens if the customer has specific conditions (2m+ posts & 500+ users) and does not have elasticsearch enabled, so it's not like that this is a performance hit for everybody, and besides that, is once a day.

I also wanted to understand what the end goal here is. Nobody reads warning logs. It's not actionable at all.

The main idea here is that since this is a WARN level, the customers alert systems will trigger an alert upon seeing this every day.

agnivade · 2023-06-22T09:21:47Z

Yeah, what I meant is that this only happens if the customer has specific conditions (2m+ posts & 500+ users) and does not have elasticsearch enabled, so it's not like that this is a performance hit for everybody, and besides that, is once a day.

Right. The thing is that if it's a small customer, then it's useless to them but we are running a query anyways. And if it's a large customer, it's a performance hit.

The main idea here is that since this is a WARN level, the customers alert systems will trigger an alert upon seeing this every day.

I want to confirm that we have data points on this and have confirmed this from multiple large customers that all of them have alert systems configured on WARN levels. AFAI am aware, large customers have alerts on system level health checks like CPU, mem etc. And they process their logs for compliance. I am not aware of any alerts set up from logs, but I might be wrong on that. Just wanted to confirm we aren't going ahead on assumptions.

I have two recommendations:

Use the statistics table to get an estimate of the post count. It won't be super accurate but it's accurate to the right order of magnitude.
Avoid this daily check, and move this to be a banner to be shown in system console when an admin logs in.

This way we avoid the perf hit and make this more visible to admins.

fmartingr · 2023-06-22T10:51:53Z

@agnivade Valid points. Let me discuss your feedback with the team.

feat: add advisor warn log for elasticsearch

cf6b901

fmartingr added the 2: Dev Review Requires review by a developer label Jun 13, 2023

fmartingr requested review from isacikgoz and agarciamontoro June 13, 2023 13:34

fmartingr self-assigned this Jun 13, 2023

mm-cloud-bot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 13, 2023

isacikgoz reviewed Jun 14, 2023

View reviewed changes

migrate to a job

5a936c9

agarciamontoro requested a review from isacikgoz June 21, 2023 12:16

agarciamontoro reviewed Jun 21, 2023

View reviewed changes

isacikgoz requested changes Jun 21, 2023

View reviewed changes

server/channels/app/server.go Outdated Show resolved Hide resolved

server/channels/app/server.go Outdated Show resolved Hide resolved

server/channels/app/server.go Outdated Show resolved Hide resolved

fmartingr and others added 3 commits June 21, 2023 15:00

Update server/channels/app/server.go

86c8c13

Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com>

Fixed job schedule

c2705cd

updated error log message

c320de6

agarciamontoro requested review from isacikgoz and agarciamontoro June 21, 2023 14:04

agarciamontoro reviewed Jun 21, 2023

View reviewed changes

server/channels/app/server.go Outdated Show resolved Hide resolved

schedule startup job to free mattermost's boot process

17bae60

agnivade reviewed Jun 22, 2023

View reviewed changes

fix: proper startup tasks

d4c9121

fmartingr marked this pull request as draft June 23, 2023 10:29

isacikgoz removed their request for review September 11, 2023 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-52867] feat: add advisor warn log for Elasticsearch #23729

[MM-52867] feat: add advisor warn log for Elasticsearch #23729

fmartingr commented Jun 13, 2023

isacikgoz left a comment

fmartingr commented Jun 15, 2023

agarciamontoro left a comment

agarciamontoro Jun 21, 2023

fmartingr Jun 21, 2023

isacikgoz left a comment

fmartingr commented Jun 21, 2023

agnivade left a comment

agnivade Jun 22, 2023

fmartingr Jun 22, 2023

agnivade commented Jun 22, 2023

fmartingr commented Jun 22, 2023 •

edited

agnivade commented Jun 22, 2023

fmartingr commented Jun 22, 2023

[MM-52867] feat: add advisor warn log for Elasticsearch #23729

Are you sure you want to change the base?

[MM-52867] feat: add advisor warn log for Elasticsearch #23729

Conversation

fmartingr commented Jun 13, 2023

Summary

Notes

Ticket Link

Release Note

isacikgoz left a comment

Choose a reason for hiding this comment

fmartingr commented Jun 15, 2023

agarciamontoro left a comment

Choose a reason for hiding this comment

agarciamontoro Jun 21, 2023

Choose a reason for hiding this comment

fmartingr Jun 21, 2023

Choose a reason for hiding this comment

isacikgoz left a comment

Choose a reason for hiding this comment

fmartingr commented Jun 21, 2023

agnivade left a comment

Choose a reason for hiding this comment

agnivade Jun 22, 2023

Choose a reason for hiding this comment

fmartingr Jun 22, 2023

Choose a reason for hiding this comment

agnivade commented Jun 22, 2023

fmartingr commented Jun 22, 2023 • edited

agnivade commented Jun 22, 2023

fmartingr commented Jun 22, 2023

fmartingr commented Jun 22, 2023 •

edited