Skip to content

Added additional env vars to control the frequency of detector activating#33

Open
symstu-tempesta wants to merge 2 commits into
mainfrom
25-new-env-vars
Open

Added additional env vars to control the frequency of detector activating#33
symstu-tempesta wants to merge 2 commits into
mainfrom
25-new-env-vars

Conversation

@symstu-tempesta

Copy link
Copy Markdown
Contributor

No description provided.

@symstu-tempesta
symstu-tempesta force-pushed the 25-new-env-vars branch 3 times, most recently from aca57a6 to f73cced Compare May 25, 2026 13:19

@krizhanovsky krizhanovsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of comments, where I think we should do different things

Comment thread utils/access_log.py Outdated
Comment thread example.env Outdated

@krizhanovsky krizhanovsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still questions on the implementation

Comment thread core/lifespan.py
Comment thread detectors/base.py

:param current_time: used as the current time in functional tests
:param interval: used as the current time in functional tests
:param previous_window_interval: control group of the access log data

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "control" means "training" or "history"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is shared across all data fetching, so it depends on the context. However, the "control group" always consists of much older data, and we assume that this data should be correct and free of traffic anomalies

Comment thread example.env
BLOCKING_NEW_WINDOW_OFFSET_SEC=10

# Duration (in seconds) of the **new** (current) window.
BLOCKING_NEW_WINDOW_DURATION_SEC=10

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these 4 new parameters? #25 requires only 2 new ones: BLOCKING_CHECK_TIMEOUT_SEC and TRAINING_MODE_HISTORY_OFFSET_MIN.

Also please write more detailed commit messages. E.g. 3890839 provides only a brief "additional variable to control data collecting intervals" and doesn't answer why do we need these 4 new parameters. There are also no comments in the code or config example, which also describes the parameters as "new" and "previous" but doesn't explain when and why the should be configured.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning to squash all commits later and add a more detailed description there, as this is a continuation of the changes and it's much easier to track them this way. The last changes I've made according to the manual on our web-site.

The example we discussed included gaps between windows — for example, the same day one week ago. In that case, we need to control the gap duration, the window duration, and its start time. While working with these parameters, I realized it would also be useful to give maximum control over the windows and make it more straightforward.

Now it is possible to configure each window independently, including its start time and duration — for example, a window that started 2 hour ago and lasts for 1 hour, and another window that started a week ago and lasts for 1 day.

symstu added 2 commits July 6, 2026 12:34
It is now possible to set the frequency of fetching data from ClickHouse and configure the fetching queries more accurately. For instance, it's good to fetch data for 1 day from a day before and one week before.

#25
Google has migrated the list of crawlers to the new url addresses
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