feat: allow configuring crawler statistics #2213

foxt451 · 2023-11-30T14:17:08Z

Allow disabling automatic persistence in Statistics and SessionPool.
Add additional methods for manual disabling in case it's needed, but just not the automatic one
Closes #1789

…ance

packages/core/src/crawlers/statistics.ts

B4nan · 2023-12-07T14:15:47Z

packages/core/src/crawlers/statistics.ts

+    /**
+     * Manually reset KV store entry for the statistics, ignoring the {@apilink StatisticsOptions.enablePersistence} flag
+     */
+    async resetStoreForce() {


do we need a separate method for this? i'd probably prefer force parameter here

B4nan · 2023-12-07T14:16:26Z

packages/core/src/crawlers/statistics.ts

+    /**
+     * Same as {@apilink Statistics.persistState}, but ignores the {@apilink StatisticsOptions.enablePersistence} flag
+     */
+    async persistStateForce() {


metalwarrior665

Just Banan's comments

Co-authored-by: Martin Adámek <banan23@gmail.com>

B4nan · 2023-12-07T16:03:47Z

packages/core/src/crawlers/statistics.ts

@@ -159,17 +159,15 @@ export class Statistics {
        this._teardown();
    }

-    async resetStore() {
-        if (!this.enablePersistence) {
+    async resetStore(opts?: {


please create a new interface for this

packages/core/src/crawlers/statistics.ts

B4nan · 2023-12-07T16:05:12Z

packages/core/src/crawlers/statistics.ts

-    async persistState() {
-        // this might be called before startCapturing was called without using await, should not crash
-        if (!this.enablePersistence || !this.keyValueStore) {
+    async persistState(opts?: {


same here, you can use the same interface (try to find some generic name that would fit both methods)

Yeah, added. I'm not really sure though whether it would be better to add the interface locally to each file or create a shared one

packages/core/src/crawlers/statistics.ts

B4nan · 2023-12-07T16:06:15Z

packages/core/src/crawlers/statistics.ts

-        if (!this.enablePersistence) {
+    async resetStore(opts?: {
+        /**
+         * If true, manually reset KV store entry for the statistics, ignoring the {@apilink StatisticsOptions.enablePersistence} flag


I am not sure about the word "manually" here. If you call this method, its always manual usage, regardless of the options.

B4nan · 2023-12-07T16:06:35Z

packages/core/src/session_pool/session_pool.ts

@@ -309,17 +307,15 @@ export class SessionPool extends EventEmitter {
        return this._createSession();
    }

-    async resetStore() {
-        if (!this.enablePersistence) {
+    async resetStore(opts?: {


B4nan · 2023-12-07T16:06:40Z

packages/core/src/session_pool/session_pool.ts

@@ -339,17 +335,15 @@ export class SessionPool extends EventEmitter {
     * Persists the current state of the `SessionPool` into the default {@apilink KeyValueStore}.
     * The state is persisted automatically in regular intervals.
     */
-    async persistState(): Promise<void> {
-        if (!this.enablePersistence) {
+    async persistState(opts?: {


packages/core/src/session_pool/session_pool.ts

Co-authored-by: Martin Adámek <banan23@gmail.com>

B4nan · 2023-12-07T18:58:43Z

packages/core/src/crawlers/statistics.ts

-    }) {
-        if (!this.enablePersistence && !opts?.force) {
+    async resetStore(opts?: PersistenceOptionsOverrides) {
+        if (!(this.enablePersistence || opts?.enablePersistence)) {


please apply this change to all the occurrences

Suggested change

if (!(this.enablePersistence || opts?.enablePersistence)) {

if (!this.enablePersistence && !opts?.enablePersistence) {

B4nan

just few comments for now

B4nan · 2023-12-07T18:59:50Z

packages/core/src/crawlers/statistics.ts

-        force?: boolean;
-    }) {
-        if (!this.enablePersistence && !opts?.force) {
+    async resetStore(opts?: PersistenceOptionsOverrides) {


lets not use abbreviations unless needed (e.g. when there is a collision)

Suggested change

async resetStore(opts?: PersistenceOptionsOverrides) {

async resetStore(options?: PersistenceOptionsOverrides) {

B4nan · 2023-12-11T08:37:33Z

packages/core/src/crawlers/statistics.ts

+    async resetStore(opts?: PersistenceOptionsOverrides) {
+        if (!this.enablePersistence && !opts?.enablePersistence) {
+            return;
+        }


i like to keep one blank line after control statements like if/for/while/etc, this applies to other places in the PR too (e.g. the persistState method), take it as a rule of thumb

Suggested change

}

}

B4nan · 2023-12-11T11:04:04Z

packages/core/src/session_pool/session_pool.ts

+/**
+ * Override persistence-related options provided in {@apilink SessionPoolOptions} for a single method call
+ */
+interface PersistenceOptionsOverrides {


I wanted to have a single interface for both since it's exactly the same. Let's export it and reuse it instead of creating two local implementations.

I also don't like the name very much, will try to come up with something else, I don't have any ideas right now :]

I've restructured it a bit, so that the same interface is used in the constructor and options parameter. Does that look okay?

I'm also not sure where to actually put this interface in the project hierarchy. I don't see a matching module for utilities/types shared by both statistics and session pool

B4nan · 2023-12-20T09:59:50Z

FYI the tests are failing, so if you are waiting for my review - its actually me who is waiting for you to fix them first :]

B4nan

thanks!

foxt451 added 3 commits November 30, 2023 16:11

feat(core): allow persistence disabling in Statistics

6b9cd5a

feat(core): allow persistence disabling for SessionPool

99c1880

feat(basic-crawler): customizable options for created Statistics inst…

9f4e015

…ance

foxt451 requested a review from B4nan November 30, 2023 14:17

foxt451 changed the title ~~Feat/statistics config~~ feat/statistics config Nov 30, 2023

foxt451 changed the title ~~feat/statistics config~~ feat/statistics-config Nov 30, 2023

foxt451 changed the title ~~feat/statistics-config~~ feat(core,basic-crawler)/statistics-config Nov 30, 2023

foxt451 changed the title ~~feat(core,basic-crawler)/statistics-config~~ feat(core,basic-crawler): statistics-config Nov 30, 2023

B4nan changed the title ~~feat(core,basic-crawler): statistics-config~~ feat: allow configuring crawler statistics Dec 7, 2023

B4nan requested a review from metalwarrior665 December 7, 2023 14:12

B4nan reviewed Dec 7, 2023

View reviewed changes

metalwarrior665 approved these changes Dec 7, 2023

View reviewed changes

foxt451 and others added 2 commits December 7, 2023 17:38

Update packages/core/src/crawlers/statistics.ts

0adb7d6

Co-authored-by: Martin Adámek <banan23@gmail.com>

fix: remove *force functions in favor of parameters

54a752f

foxt451 requested a review from B4nan December 7, 2023 15:57

B4nan reviewed Dec 7, 2023

View reviewed changes

B4nan requested changes Dec 7, 2023

View reviewed changes

foxt451 and others added 5 commits December 7, 2023 18:08

Update packages/core/src/crawlers/statistics.ts

1ca2536

Co-authored-by: Martin Adámek <banan23@gmail.com>

Update packages/core/src/crawlers/statistics.ts

8f3a10e

Co-authored-by: Martin Adámek <banan23@gmail.com>

Update packages/core/src/session_pool/session_pool.ts

8da3b0c

Co-authored-by: Martin Adámek <banan23@gmail.com>

Update packages/core/src/session_pool/session_pool.ts

5f8c3b6

Co-authored-by: Martin Adámek <banan23@gmail.com>

feat: add PersistenceOptionsOverrides

a4eca60

B4nan reviewed Dec 7, 2023

View reviewed changes

chore: fix condition style

a611e07

foxt451 requested a review from B4nan December 8, 2023 11:34

mtrunkat added the t-tooling Issues with this label are in the ownership of the tooling team. label Dec 11, 2023

B4nan reviewed Dec 11, 2023

View reviewed changes

fix: some PR remarks

161c185

foxt451 requested a review from B4nan December 12, 2023 18:04

fix: enable persistence by default

fc0b44d

foxt451 requested review from B4nan and removed request for B4nan December 20, 2023 13:57

B4nan approved these changes Dec 20, 2023

View reviewed changes

B4nan merged commit 9fd60e4 into master Dec 20, 2023
8 checks passed

B4nan deleted the feat/statistics-config branch December 20, 2023 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow configuring crawler statistics #2213

feat: allow configuring crawler statistics #2213

foxt451 commented Nov 30, 2023

B4nan Dec 7, 2023

B4nan Dec 7, 2023

metalwarrior665 left a comment

B4nan Dec 7, 2023

B4nan Dec 7, 2023

foxt451 Dec 7, 2023

B4nan Dec 7, 2023

B4nan Dec 7, 2023

B4nan Dec 7, 2023

B4nan Dec 7, 2023

B4nan left a comment

B4nan Dec 7, 2023

B4nan Dec 11, 2023

B4nan Dec 11, 2023

foxt451 Dec 12, 2023

B4nan commented Dec 20, 2023

B4nan left a comment

	if (!(this.enablePersistence \|\| opts?.enablePersistence)) {
	if (!this.enablePersistence && !opts?.enablePersistence) {

	async resetStore(opts?: PersistenceOptionsOverrides) {
	async resetStore(options?: PersistenceOptionsOverrides) {

feat: allow configuring crawler statistics #2213

feat: allow configuring crawler statistics #2213

Conversation

foxt451 commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metalwarrior665 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

B4nan 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

B4nan commented Dec 20, 2023

B4nan left a comment

Choose a reason for hiding this comment