Server: Resolves #9931: Add task to delete events older than a week #11372

AdrienPoupa · 2024-11-11T01:14:39Z

Revisiting https://github.com/laurent22/joplin/pull/9988/files as I had the same issue this week (my Joplin DB grew to 600mb while I only have a few notes...).

I'm using mostly the same code, but I tried to match other patterns found in the codebase (using a TTL attribute in the class, not requiring arguments in the event function).

I also handled Laurent's review to use fake timers for the test.

github-actions · 2024-11-11T01:14:52Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

AdrienPoupa · 2024-11-11T01:15:35Z

I have read the CLA Document and I hereby sign the CLA

laurent22

Thanks for looking into this. I think there's two issues:

The TTL should be configurable via an env variable
What's the impact of deleting old events - what service or function could be affected by it?

laurent22 · 2024-11-11T12:49:41Z

packages/server/src/models/EventModel.ts

@@ -4,6 +4,8 @@ import BaseModel, { UuidType } from './BaseModel';

 export default class EventModel extends BaseModel<Event> {

+	private eventTtl_: number = 7 * 24 * 60 * 60 * 1000;


7 * Day is clearer (import the Day constant)

I was using TokenModel as a reference that has the exact same line. I changed it there as well.

packages/server/src/models/EventModel.test.ts

AdrienPoupa · 2024-11-11T15:45:32Z

Hi Laurent, thanks for the review.

The TTL should be configurable via an env variable

What would we gain from this? The DeleteExpiredTokens task that I'm using as a reference does not have this logic.

If we need to implement it, do you have a preferred approach? I see the UserDeletionService uses the USER_DATA_AUTO_DELETE_AFTER_DAYS environment variable, I could use the same mechanism.

What's the impact of deleting old events - what service or function could be affected by it?

I don't know, sorry. It is the approach you suggested here however: #9931 (comment)

laurent22 · 2024-11-11T18:39:56Z

What would we gain from this?

For audit purposes for example.

I don't know, sorry. It is the approach you suggested here however: #9931 (comment)

If you can check please post here

AdrienPoupa · 2024-11-12T00:58:50Z

I analyzed events and their usages, I found that we're creating events in TaskService's runTask just before and after the task is handled:

await this.models.event().create(EventType.TaskStarted, id.toString());

and

await this.models.event().create(EventType.TaskCompleted, id.toString());

In taskLastEvents, we retrieve the last events for a given task:

	public async taskLastEvents(id: TaskId): Promise<TaskEvents> {
		return {
			taskStarted: await this.models.event().lastEventByTypeAndName(EventType.TaskStarted, id.toString()),
			taskCompleted: await this.models.event().lastEventByTypeAndName(EventType.TaskCompleted, id.toString()),
		};
	}

This function, in turn, is used in the admin/tasks route to show task events (columns Last Run and Last Completion).

Those were the only usages I could find, so they're here for auditing/debugging purposes, they have no impact on the behaviour of the application.

AdrienPoupa · 2024-11-12T01:48:15Z

I added two environment variables to enable the deletion and control the cut off period, and improved the test coverage. Hopefully that should do it :)

laurent22

Thank you for looking into this and for adding the two env variables and test.

For now please set it to disabled by default and default duration to 30 days. This is because even though today we don't need to keep them for a long time, in the future there could be a service that needs them for a longer time, and users who will have it configured to 7 days might have issues.

What's the meaning of setting EVENTS_AUTO_DELETE_AFTER_DAYS to 0? It deletes everything immediately?

laurent22 · 2024-11-16T21:39:28Z

packages/server/src/models/EventModel.ts

+	private readonly eventTtl_: number;
+	private readonly eventAutoDelete_: boolean;
+
+	public constructor(db: DbConnection, dbSlave: DbConnection, modelFactory: NewModelFactoryHandler, config: Config) {
+		super(db, dbSlave, modelFactory, config);
+		this.eventAutoDelete_ = config.EVENTS_AUTO_DELETE_ENABLED;
+		this.eventTtl_ = config.EVENTS_AUTO_DELETE_AFTER_DAYS * Day;
+	}


Actually the config is available from TaskService, so could you please do the following:

Remove the eventTtl_ and eventAutoDelete_ from here

Make deleteOldEvents take the TTL as argument

In TaskService, only enable the task if EVENTS_AUTO_DELETE_ENABLED is true (same as USER_DATA_AUTO_DELETE_ENABLED)

Still from TaskService pass EVENTS_AUTO_DELETE_AFTER_DAYS * Day as argument

This is to make the way it works more explicit and to avoid having to modify a global object (the config) in tests, which can have unexpected results.

In TaskService, only enable the task if EVENTS_AUTO_DELETE_ENABLED is true (same as USER_DATA_AUTO_DELETE_ENABLED)

That was already the case.

I made the changes you requested but had to remove a few tests that no longer made sense.

laurent22 · 2024-11-16T21:40:07Z

packages/server/src/models/TokenModel.ts

+import { Day } from '../utils/time';

 export default class TokenModel extends BaseModel<Token> {

-	private tokenTtl_: number = 7 * 24 * 60 * 60 * 1000;
+	private tokenTtl_: number = 7 * Day;


Please remove this - I agree it's ok as a change but I prefer to keep commits focused on one specific issue.

laurent22 · 2024-11-16T21:40:55Z

packages/server/src/models/EventModel.ts

+		return this.withTransaction(async () => {
+			await this.db(this.tableName)
+				.where('created_time', '<', cutOffDate)
+				.delete();
+		}, 'EventModel::deleteOldEvents');


No need for withTransaction since there's only one db call

And the function shouldn't return a value

AdrienPoupa · 2024-11-16T22:40:28Z

What's the meaning of setting EVENTS_AUTO_DELETE_AFTER_DAYS to 0? It deletes everything immediately?

It will delete events older than the current timestamp, so yes. I don't think that's an issue given you'd have to go ahead and actively set the config as such.

laurent22

Thanks @AdrienPoupa, I've just left one last comment and then we can merge

packages/server/src/models/EventModel.ts

… a week

github-actions bot added a commit that referenced this pull request Nov 11, 2024

@AdrienPoupa has signed the CLA in #11372

63fce9c

AdrienPoupa force-pushed the feat/cleanup-events branch from 0ada87b to 1d3dfca Compare November 11, 2024 01:25

laurent22 reviewed Nov 11, 2024

View reviewed changes

AdrienPoupa force-pushed the feat/cleanup-events branch from a796af6 to c32076b Compare November 12, 2024 01:49

laurent22 requested changes Nov 16, 2024

View reviewed changes

laurent22 reviewed Nov 27, 2024

View reviewed changes

packages/server/src/models/EventModel.ts Outdated Show resolved Hide resolved

Server: Resolves laurent22#9931: Add task to delete events older than…

cc34ff8

… a week

AdrienPoupa force-pushed the feat/cleanup-events branch from e191d09 to cc34ff8 Compare November 30, 2024 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server: Resolves #9931: Add task to delete events older than a week #11372

Server: Resolves #9931: Add task to delete events older than a week #11372

AdrienPoupa commented Nov 11, 2024

github-actions bot commented Nov 11, 2024 •

edited

Loading

AdrienPoupa commented Nov 11, 2024

laurent22 left a comment

laurent22 Nov 11, 2024

AdrienPoupa Nov 11, 2024

AdrienPoupa commented Nov 11, 2024 •

edited

Loading

laurent22 commented Nov 11, 2024

AdrienPoupa commented Nov 12, 2024

AdrienPoupa commented Nov 12, 2024

laurent22 left a comment

laurent22 Nov 16, 2024

AdrienPoupa Nov 16, 2024 •

edited

Loading

laurent22 Nov 16, 2024

AdrienPoupa Nov 16, 2024

laurent22 Nov 16, 2024

laurent22 Nov 16, 2024

AdrienPoupa Nov 16, 2024

AdrienPoupa commented Nov 16, 2024

laurent22 left a comment

		@@ -4,6 +4,8 @@ import BaseModel, { UuidType } from './BaseModel';

		export default class EventModel extends BaseModel<Event> {

		private eventTtl_: number = 7 * 24 * 60 * 60 * 1000;

Server: Resolves #9931: Add task to delete events older than a week #11372

Are you sure you want to change the base?

Server: Resolves #9931: Add task to delete events older than a week #11372

Conversation

AdrienPoupa commented Nov 11, 2024

github-actions bot commented Nov 11, 2024 • edited Loading

AdrienPoupa commented Nov 11, 2024

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdrienPoupa commented Nov 11, 2024 • edited Loading

laurent22 commented Nov 11, 2024

AdrienPoupa commented Nov 12, 2024

AdrienPoupa commented Nov 12, 2024

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdrienPoupa Nov 16, 2024 • edited Loading

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

AdrienPoupa commented Nov 16, 2024

laurent22 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 11, 2024 •

edited

Loading

AdrienPoupa commented Nov 11, 2024 •

edited

Loading

AdrienPoupa Nov 16, 2024 •

edited

Loading