-
-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add usage related stuff + fix popularity related stuff #35
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 98.18% 98.35% +0.16%
==========================================
Files 30 34 +4
Lines 936 1031 +95
Branches 181 195 +14
==========================================
+ Hits 919 1014 +95
Misses 12 12
Partials 5 5 ☔ View full report in Codecov by Sentry. |
40bfd62
to
1b2327b
Compare
1b2327b
to
e8fb4f5
Compare
@rgaudin did you already start reviewing this PR or may I push a new commit? (almost negligible change, but still something you might have already spotted / might confuse you) |
Haven't started ; please do |
d7a0078
to
d8dfaa1
Compare
Ready again for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Wise renaming while it's still doable 😄
What's the storage impact of this new generic PackageRequest?
backend/src/offspot_metrics_backend/business/indicators/__init__.py
Outdated
Show resolved
Hide resolved
backend/src/offspot_metrics_backend/business/indicators/__init__.py
Outdated
Show resolved
Hide resolved
backend/src/offspot_metrics_backend/business/indicators/recorder.py
Outdated
Show resolved
Hide resolved
backend/src/offspot_metrics_backend/business/kpis/total_usage.py
Outdated
Show resolved
Hide resolved
Inputs are not stored, so there is 0 impact on storage. There is a computing impact since these inputs will have to be processed, but since most indicators do not process this kind of inputs (only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ; those comments are really helpful
Rationale
Fix #32
#33 is almost fixed but there is still an open question in the issue which needs to be tackled before closing
Fix #34
NOTA: some changes most probably broke the UI (changes in KPI 2001 and 2002 data structures), but since the UI will be completely revamped anyway, I do not lost my time fixing this for now, we can live with this until I restart working on the UI.
Changes
UsageRecorder
to count active slots of 10 minutes during which an input has been receivedCommonInputGenerator
for cases not already covered by existing generators (apps which are not edupi)PackageRequest
input to indicate that web traffic has been received on a given package and generate this input on all generators, in addition to existing inputsfrozen
andeq
, so that we can compare them more easily (and they must be frozen anyway)TotalUsage
KPI for 2003