stats: add stats to track ILSRecord insertions, deletions and updates #1251
Conversation
JakobMiesner
force-pushed
the
feature/stats/ils-record-changes
branch
from
03afa7b to
6f301b0
Compare
JakobMiesner
force-pushed
the
feature/stats/ils-record-changes
branch
4 times, most recently
from
1d276c2 to
c6d53fa
Compare
| pid_type = record._pid_type | ||
|
|
||
| event_data = { | ||
| "timestamp": datetime.datetime.now().isoformat(), |
There was a problem hiding this comment.
we normally store dates in UTC timezone, without timezone info. The client can then transform it.
I don't exactly recall, is now() using the server timezone?
There was a problem hiding this comment.
you are right, thanks for the catch
and now is using the server timezone, yes. See here
JakobMiesner
force-pushed
the
feature/stats/ils-record-changes
branch
from
c6d53fa to
484ad86
Compare
JakobMiesner
force-pushed
the
feature/stats/ils-record-changes
branch
from
484ad86 to
cde2123
Compare
invenio_app_ils/stats/processors.py
Outdated
| """Add unique_id and aggregation_id to the doc.""" | ||
|
|
||
| # We use this field to group by during aggregation. | ||
| # e.g. the count of created eitems by a user with id 7 is tracked under eitmid__insert__None. |
There was a problem hiding this comment.
only one minor thing....
eitmid__insert__None the None part I find it a bit strange. Is this avoidable?
There was a problem hiding this comment.
For changes not performed by users or by users that have no backoffice access the user id will be None. (please look here for a more detailed description)
I could do a check in the aggregation_id creation and replace the None with something like no_user_id so it would be eitmid__insert__no_user_id, but I dont really find it much clearer. And in the end the aggreagtion_id is only used for aggregation and will not really be interacted with.
There was a problem hiding this comment.
could it be just etmid_insert instead? also... it should never happen that a non-librarian user creates any record except a document request. It seems like an overkill to have the <>_None pattern in the first place
There was a problem hiding this comment.
@kpsherva sounds good. I changed it :) I think the non-librarian record change can happen when e.g. a celery job changes any record. I don't think that the global current_user would evaluate to anything in that case.
There was a problem hiding this comment.
also importer comes to mind, could happen in the importer. if it is none, maybe we should have value "script/importer"?
ntarocco
left a comment
ntarocco
left a comment
There was a problem hiding this comment.
There is still a comment from Karolina, otherwise LGTM
3 participants
closes: CERNDocumentServer/cds-ils#1031