rehanvdm / serverless-website-analytics

A CDK construct that consists of a serverless backend, frontend and client side code to track website analytics
GNU General Public License v2.0
167 stars 13 forks source link

Udpate Anomaly Detection Algorithm so that it better detects when an anomaly is over #75

Open rehanvdm opened 7 months ago

rehanvdm commented 7 months ago

Currently, an anomaly is marked as over: the first time a value is under the breach threshold and the first time the slope is positive.

2024-02-15 19:00 07 (17) ========== |
2024-02-15 18:00 01 (17) = #
2024-02-15 17:00 07 (19) ========== # 2024-02-15 16:00 08 (21) =========== # 2024-02-15 15:00 17 (21) ========================
2024-02-15 14:00 21 (20) =============================# 2024-02-15 13:00 20 (19) ===========================#=

The above example is of a slow breach, there we could say that if the value is below the threshold, then mark it as over.

But for big spikes it is a different story, test and find a solution for both scenarios. Here we are trying to find the whole event, if we just say when the value is not breaching then we loose the parts I marked in red. image

Explore having a third EMA with a much slower reaction, maybe an Alpha of 0.01, then if that is crossed after an anomaly, mark the evaluation as OK? Something like that

kc0bfv commented 5 months ago

I wonder what you think about having a static minimum threshold for anomaly detection? I don't get a ton of traffic, so if I go from 1 to 5 hits I get an anomaly email. I'd prefer to only see more problematic anomalies - like, when they're occurring at over 1000 hits.

I could bump the BREACHING_MULTIPLIER, but I think if I tuned that to the level I was looking for the anomaly would have to be very sudden and very significant, and in the event that traffic crept up before exceeding 1000/10000-ish, I wouldn't be alerted unless there was a sudden significant anomaly at that level.

I'm experimenting with adding a STATIC_THRESHOLD, below which the breachingThreshold cannot be exceeded.

rehanvdm commented 5 months ago

Yes, this will already be a good addition.

It's not the direct solution to the problem I described, but I really like the suggestion. It will reduce the number of false positives and alert fatigue

kc0bfv commented 5 months ago

This is my set of commits that do this - I think. I had the important logic backwards (silly me) and just fixed it and re-installed.

https://github.com/kc0bfv/serverless-website-analytics/compare/9e28906...kc0bfv:serverless-website-analytics:99934f1

It's a hack, but I think it's compatible with the types of changes you might be interested in for the future. If you don't hate what I did there, I can make it a pull request.

rehanvdm commented 5 months ago

I think that looks about right.

I would rather that instead of clamping the breaching threshold we just change the boolean that is evaluated here, so that it becomes:

const breachingLatest = latestRecord.views > LambdaEnvironment.STATIC_THRESHOLD && latestRecord.views > breachingThreshold

I also think that minimumViews is a better description than staticThreshold.

Yeah, the whole anomaly detection is a bit DIY, not sure if you found https://github.com/kc0bfv/serverless-website-analytics/blob/main/docs/ANOMALY_DETECTION.md#faq yet? The FAQ explains a bit about the why. I am open for other suggestions that is less DIY/maintenance and still fit within the scope of the project. Basically, I don't want to introduce containers, so the libraries I used in Python were too big for Lambda which forces the use of containers.

kc0bfv commented 5 months ago

I'm make those changes soon and make a PR!

On Mon, Apr 29, 2024, 00:22 Rehan van der Merwe @.***> wrote:

I think that looks about right.

I would rather that instead of clamping the breaching threshold we just change the boolean that is evaluated here https://github.com/kc0bfv/serverless-website-analytics/compare/9e28906...kc0bfv:serverless-website-analytics:99934f1#diff-22c9af3669ed3a168c9fba7b53904735936f21bc4f609eb9d2c2439114069cc9R175, so that it becomes:

const breachingLatest = latestRecord.views > LambdaEnvironment.STATIC_THRESHOLD && latestRecord.views > breachingThreshold

I also think that minimumViews is a better description than staticThreshold.

Yeah, the whole anomaly detection is a bit DIY, not sure if you found https://github.com/kc0bfv/serverless-website-analytics/blob/main/docs/ANOMALY_DETECTION.md#faq yet? The FAQ explains a bit about the why. I am open for other suggestions that is less DIY/maintenance and still fit within the scope of the project. Basically, I don't want to introduce containers, so the libraries I used in Python were too big for Lambda which forces the use of containers.

— Reply to this email directly, view it on GitHub https://github.com/rehanvdm/serverless-website-analytics/issues/75#issuecomment-2081909270, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALGLJIOB6NPICZPVLRIPYLY7XKJZAVCNFSM6AAAAABDLXRDQOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBRHEYDSMRXGA . You are receiving this because you commented.Message ID: @.***>

kc0bfv commented 5 months ago

I think the general idea of your anomaly detection code makes sense and is interesting - it is DIY but it seems good to me. The logic isn't super complex like crypto or something, so being DIY in this case seems fine.

Variable name change made, PR made.