Annotations: Clean up old annotations job #20815

malish8632 · 2019-12-02T20:15:23Z

What this PR does / why we need it:
This adds automatic clean up job to remove expired annotations and fixes existing issue #8224
Current number of days back to clean annotations is set to 60 and could be updated in defaults.ini settings.
This PR is not distinguishing between types of annotations. Any alerts or user created annotations treated the same.
This PR is also increasing the size of annotation.tags field as we encountered issues with VARCHAR(500) as a limit on our instance.

Which issue(s) this PR fixes:

Fixes #8224

Special notes for your reviewer:

malish8632 · 2020-01-02T19:06:35Z

Hej @marefr any feedback here?

aknuds1 · 2020-01-29T14:07:47Z

Thanks for the PR, I will have a look at it.

aknuds1

After my revisions to this PR, it looks good to me. I need input from others on the team though.

conf/defaults.ini

conf/sample.ini

pkg/models/annotations.go

pkg/services/cleanup/cleanup.go

pkg/services/sqlstore/annotations_cleanup.go

bergquist · 2020-02-06T13:26:11Z

pkg/setting/setting.go

+	// read dashboard settings
+	annotations := iniFile.Section("annotations")
+	DaysToKeepAnnotations = annotations.Key("days_to_keep").MustInt(60)
+	if DaysToKeepAnnotations < 5 {


Any thoughts about this? Why not below 5? :)

I'm actually unsure about this too @bergquist - 5 was the minimum before my revising the PR.

bergquist · 2020-02-06T13:28:44Z

conf/defaults.ini

+
+[annotations]
+# Number of days to keep annotations (per alert). Default: 60, Minimum: 5
+days_to_keep = 60


I'm torn about having this on by default.

@bergquist I guess it should be turned off by default?

I agree! At least now when it operates on all kinds of annotations. Maybe cleanup of alert annotations enabled per default would have made sense if it was the default from the beginning when alerting was introduced, but now it's sort of a breaking change so think we have to have it disabled per default.

bergquist · 2020-02-06T16:18:17Z

conf/defaults.ini

+#################################### Annotations ##################
+
+[annotations]
+# Number of days to keep annotations (per alert). Default: 60, Minimum: 5


This comment says per alert but the SQL statement deletes all kinds of annotations. I think we should be clear about this and perhaps have different settings per kind of annotation.

@bergquist Thanks for clarifying that 👍

marefr

Some smaller things, see comments. In addition, configuration documentation needs to be updated as well.

marefr · 2020-02-06T22:55:56Z

pkg/services/sqlstore/migrations/annotation_mig.go

@@ -145,6 +145,11 @@ func addAnnotationMig(mg *Migrator) {
 	mg.AddMigration("Remove index org_id_epoch_epoch_end from annotation table", NewDropIndexMigration(table, &Index{
 		Cols: []string{"org_id", "epoch", "epoch_end"}, Type: IndexType,
 	}))
+
+	// Increase size of tags field in annotation table


I would suggest moving this change to a separate PR with a referenced issue (existing or new one). Easier to keep each change separate and in case this would cause problem it's easier to revert only this instead of every change in this PR.

marefr · 2020-02-06T22:59:17Z

pkg/services/sqlstore/annotations_cleanup_test.go

+	daysToKeepAnnotations := 5
+	repo := SqlAnnotationRepo{}
+
+	Convey("Deletion of expired annotations", t, func() {


New tests should use standard library for testing, see backend styleguide.

@malish8632 Can you rewrite the new tests with the standard library as @marefr requests?

hi @aknuds1 I'm going to take a stab at this.

Thanks @malish8632!

@aknuds1 @marefr updated to use standard library

marefr · 2020-02-06T23:04:24Z

conf/defaults.ini

+
+[annotations]
+# Number of days to keep annotations (per alert). Default: 60, Minimum: 5
+days_to_keep = 60


I agree! At least now when it operates on all kinds of annotations. Maybe cleanup of alert annotations enabled per default would have made sense if it was the default from the beginning when alerting was introduced, but now it's sort of a breaking change so think we have to have it disabled per default.

marefr · 2020-02-06T23:08:23Z

pkg/services/sqlstore/annotations_cleanup.go

+			return nil
+		}
+
+		query = fmt.Sprintf("DELETE FROM annotation_tag WHERE annotation_id IN (?%s)", strings.Repeat(",?",


Are we comfortable/sure this will not lock the table and interfere with alerting?

This was build with approach identical to how cleaning of dashboard_version table is done, which is currently the biggest in our internal system. Cleaning annotations is never being an issue for our MySQL server.

marefr · 2020-02-06T23:08:42Z

pkg/services/sqlstore/annotations_cleanup.go

+	bus.AddHandler("sql", deleteExpiredAnnotations)
+}
+
+const MAX_EXPIRED_ANNOTATIONS_TO_DELETE = 900


I'm also unsure about that @marefr. Maybe we should derive a sensible number?

Initially tried 1000 but due to unit tests failures changed to 900

malish8632 · 2020-02-21T18:11:57Z

@aknuds1 For some reason grafana-docker-ubuntu-pr is failing... any pointers?

aknuds1 · 2020-02-21T19:58:34Z

Please merge with grafana master to fix build issues.

…

On Fri, Feb 21, 2020, 19:11 Sergey Rustamov ***@***.***> wrote: @aknuds1 <https://github.com/aknuds1> For some reason grafana-docker-ubuntu-pr is failing... any pointers? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20815?email_source=notifications&email_token=AACEVV43MGWVJ72N2UR7TZDREAKO5A5CNFSM4JUFVHV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMTSXCI#issuecomment-589769609>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEVVZ57J5CHW54JGUWMGLREAKO5ANCNFSM4JUFVHVQ> .

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

CLAassistant · 2020-03-03T15:51:24Z

All committers have signed the CLA.

malish8632 · 2020-03-03T15:54:52Z

@aknuds1 Is there anything else you guys expecting from me? Thanks

aknuds1 · 2020-03-09T16:10:04Z

@malish8632 I requested new reviews from @bergquist and @marefr, let's see what they say. You also need to fix conflicts :)

bergquist · 2020-03-18T10:19:29Z

pkg/services/sqlstore/annotations_cleanup.go

+const MAX_EXPIRED_ANNOTATIONS_TO_DELETE = 900
+
+func deleteExpiredAnnotations(cmd *m.DeleteExpiredAnnotationsCommand) error {
+	return inTransaction(func(sess *DBSession) error {


Return this as inTransaction performs all these SQL operations in one transaction. I dont think that's necessary. You can use withDbSession instead to avoid transactions.

stale · 2020-04-01T10:58:36Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

tomtastic · 2020-04-11T11:18:53Z

Looking forward to seeing this merged.

stale · 2020-04-25T11:38:07Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-06-08T10:47:10Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-06-22T11:27:26Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-07-23T12:56:03Z

This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

tomtastic · 2020-07-23T13:52:09Z

Wow, stale bots suck.

marefr · 2020-08-20T13:05:33Z

@tomtastic We've opened a new PR based on discussions in related issue

marefr added area/backend pr/external This PR is from external contributor labels Dec 4, 2019

aknuds1 self-assigned this Jan 29, 2020

aknuds1 self-requested a review January 29, 2020 14:07

aknuds1 added this to Under review in Backend Platform Squad Jan 29, 2020

aknuds1 approved these changes Feb 6, 2020

View reviewed changes

aknuds1 requested a review from a team February 6, 2020 12:50

aknuds1 added the area/annotations label Feb 6, 2020

bergquist reviewed Feb 6, 2020

View reviewed changes

marefr requested changes Feb 6, 2020

View reviewed changes

Sergey Rustamov and others added 15 commits February 21, 2020 15:12

Clean up old annotations job

8ae2764

Unexport function

101bba6

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Fix build failures

91f7982

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Improve migration description

698c589

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Rename variables

073df95

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Minimize dependency on setting module

4fef8e1

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Clean up tests

2d7acca

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

conf: Fix annotation conf documentation

4fbdd0d

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Fix typo

f27b981

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Don't ignore error

02395d1

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Simplify

efb77ee

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Clean up tests

ac7e190

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Perform insertions in transactions

aae8394

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Simplify tests

dab5dda

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Separate tests

a85a97e

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

aknuds1 and others added 3 commits February 21, 2020 15:13

Rename constant

cb2fc1d

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

Using testify for testing

e36786e

gofmt styles fixed

c3594db

malish8632 force-pushed the feature-8224 branch from b1b4c5d to c3594db Compare February 21, 2020 20:14

aknuds1 requested review from bergquist and marefr March 9, 2020 16:09

bergquist reviewed Mar 18, 2020

View reviewed changes

stale bot added the stale Issue with no recent activity label Apr 1, 2020

stale bot removed the stale Issue with no recent activity label Apr 11, 2020

stale bot added the stale Issue with no recent activity label Apr 25, 2020

aknuds1 removed the stale Issue with no recent activity label May 25, 2020

stale bot added the stale Issue with no recent activity label Jun 8, 2020

aknuds1 removed the stale Issue with no recent activity label Jun 8, 2020

stale bot added the stale Issue with no recent activity label Jun 22, 2020

stale bot closed this Jul 23, 2020

Backend Platform Squad automation moved this from Under review to Done Jul 23, 2020

marefr removed this from Done in Backend Platform Squad Sep 7, 2020

Annotations: Clean up old annotations job #20815

Annotations: Clean up old annotations job #20815

Conversation

malish8632 commented Dec 2, 2019

malish8632 commented Jan 2, 2020

aknuds1 commented Jan 29, 2020

aknuds1 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aknuds1 Feb 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marefr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malish8632 commented Feb 21, 2020

aknuds1 commented Feb 21, 2020 via email

CLAassistant commented Mar 3, 2020

malish8632 commented Mar 3, 2020

aknuds1 commented Mar 9, 2020

Choose a reason for hiding this comment

stale bot commented Apr 1, 2020

tomtastic commented Apr 11, 2020

stale bot commented Apr 25, 2020

stale bot commented Jun 8, 2020

stale bot commented Jun 22, 2020

stale bot commented Jul 23, 2020

tomtastic commented Jul 23, 2020

marefr commented Aug 20, 2020

aknuds1 left a comment •

edited

aknuds1 Feb 6, 2020 •

edited