Issue #67: Backfilled metric - online users #92

marcghaly · 2022-05-26T18:34:26Z

Hi @jaypha, hi @brendanheywood,

This is PR for issue #67.

Regards,

Marc

classes/metric/concurrent_users_metrics.php

classes/metric/active_users_metric.php

classes/metric/concurrent_users_metrics.php

collector/database/classes/collector.php

collector/database/classes/form/metric_backfill_form.php

jaypha · 2022-05-27T00:42:05Z

There are some issues here which I have concerns about. I'll be happy to discuss if you want to.

classes/metric/concurrent_users_metrics.php

collector/database/classes/collector.php

jaypha · 2022-05-30T06:35:40Z

Hi Marc.

I don't think a meeting is necessary. You have addressed my primary concerns for now. I'm a little iffy about having it controlled from within the database collector, but that's something I'd rather leave to Brendan.

Just that one fix causing the CI problem, and then I am OK to merge. Get Brendan to have a look at the UI aspect.

Regards

classes/metric/concurrent_users_metrics.php

classes/metric/base.php

marcghaly · 2022-06-01T15:54:48Z

Hi @brendanheywood,

I have made changes for this PR, a concern that I have is the use of the API for retrieving logs that does not provide a faster / lighter way to retrieve concurrent users (ie: using DISTINCT in a SELECT statement - retrieving all data * with the 2 methods provided by the API get_events_select_iterator, get_events_select seems super heavy to me, please let me know if there is a way I am not aware of) - it seems it is taking a lot of time for this operation, if that were to be left as is, would it need a spinner display ? This is rebased against latest MOODLE_35_STABLE.
Concerning a meeting, I am available at 8 PM EST so 10 AM AEST your time, this evening and tomorrow if you want.

Regards,

Marc

brendanheywood · 2022-06-01T23:30:58Z

hi @marcghaly

If the log api is too slow then do it directly like it was. But just to be be clear @jaypha was doing / has just done a huge refactor of the way the metrics work and I didn't want to waste your time as it would affect your patch. I suspect you will need to have a look at what has landed and rewrite a lot of it (despite there being no conflicts)

We have our standup at 10, so lets chat right after that. Can you please ping me on matrix, I pinged you the other day but you didn't see it and I'm not sure I have the real 'you' in matrix

collector/database/chart.php

classes/metric/base.php

collector/database/classes/collector.php

brendanheywood · 2022-06-07T13:54:00Z

collector/database/classes/collector.php

+                // Trick to avoid erasing data when it only needs to be completed.
+                $this->record_metrics($metricitems);
+            } else {
+                $this->delete_metrics($metricclass->get_name());


I'm a little confused why we'd ever need to delete data? If we are being asked to backfill data from time period which already has data then that smells like what ever is calling record_saved_metrics isn't sending correct times

I found it confusing seeing different frequencies in the chart - what happens at the moment : it retrieves all data for the requested time-frame unless the frequency changed, when frequency changes, it deletes all data and replaces it with the new data with proper interval. Also, if I request data for 4 months, it will fill data for those 4 months then if I request data for 12 months, it will retrieve data from the lowest timestamp present in DB to the passed point time requested (with the period) - it will only retrieve the missing part.

I think deleting data has some business value but I think it should be explicit, similar to the setting for how much data you want to keep around. I don't like the idea of someone trying to add in more data and ending up losing data they already had.

I've split that off into a new issue, if you want you can tackle that after this pr lands:
#104

For this one I'd just want it to only ever add more data

I have set it up so it never deletes data now - but also that if frequency changed it is not overlapping data of another interval - It would get extremely confusing. If frequency changes, timestamp needs to be earlier than lowest one in DB so it completes data, it looks like this. Highlighted the one month period - the other one 1 week.

I will sure get on this one once this PR lands.

collector/database/classes/form/metric_backfill_form.php

brendanheywood · 2022-06-07T14:00:23Z

Just checking, in the following example it is set to be hourly, for example between two first rows, should it display between 12:00 and 16:00 (12, 13, 14, 15 , 16) with a 0 value ? Is that what you mean ? I am using pgsql maybe issue is related to mariadb/mysqli

The golden rule is imagine that the metric had been running the whole time and we didn't need to back fill. Ideally the end result should be exactly the same. So yes there would be a metric for every time slice and many of them would be 0

brendanheywood · 2022-06-07T14:00:39Z

Also I am using postgress too

marcghaly · 2022-06-07T23:09:18Z

Hi @brendanheywood,

I have made changes according to your comments, If changes are okay I will squash commits.

Cheers,

Marc

classes/metric/base.php

marcghaly · 2022-06-10T21:26:51Z

Hi @brendanheywood, this is squashed and rebased over latest master.

Regards,

Marc

brendanheywood · 2022-06-20T13:45:39Z

thanks @marcghaly

marcghaly requested a review from jaypha May 26, 2022 18:34

jaypha reviewed May 26, 2022

View reviewed changes