astrosat / django-astrosat-core

Common backend library for Astrosat projects' core functionality
GNU General Public License v3.0
0 stars 0 forks source link

add support to POST to the logging view #33

Closed allynt closed 3 years ago

allynt commented 4 years ago

Describe the feature

We can already manually create log entries in the backend. But a way to create them via API from the frontend may be needed.

allynt commented 4 years ago

The core of the logging functionality is here:

https://github.com/astrosat/django-astrosat-core/blob/f5a49ce913246b6767a7fc40343b71c458b3b3cb/astrosat/utils/utils_logging.py#L31-L63

As you can see, I've created a logger that sends messages to the db.

allynt commented 4 years ago

The existing view:

https://github.com/astrosat/django-astrosat-core/blob/f5a49ce913246b6767a7fc40343b71c458b3b3cb/astrosat/views.py#L242-L248

should be changed from a ReadOnlyModelViewset to separate generic ListCreateView [https://www.django-rest-framework.org/api-guide/generic-views/]; this will let us create a new log record and retrieve existing log records (but not update nor delete records). You probably want to change the permssion/serializer based on whether we're doing a POST or a GET.

For the pemission I would make a new PermissionClass [https://www.django-rest-framework.org/api-guide/permissions/#custom-permissions] such that SAFE_METHODS can be accessed when DEBUG is True, or the Admin is making the request, and all other methods the user must be authenticated. This means that any logged in user can create a LogRecord, but only the admin (or any user if debug=true) can view LogRecords.

For the serializer, you can probably do something clever by overriding the get_serializer method. (This is untested, a lot of documentation will recommend that you override the get_serializer_class method - but you want to use the same underlying class in both "create" and "list" cases, it's just that in the former you want to use the built in ListSerializer class to wrap it which automatically gets instanciated if many=true is passed to the serializer):

    def get_serializer(self, *args, **kwargs):
        if self.action == 'post':
            data = kwargs.get('data')
            kwargs['many'] = isinstance(data, list)
        return super().get_serializer(*args, **kwargs)

You could also try to do clever stuff w/ partial [https://docs.python.org/3/library/functools.html#functools.partial] to pre-populate the DatabaseLogRecordSerializer class w/ many=true if action==create.

However, none of the above may actually be relevant b/c you want to override the create method to not create objects using standard DRF seriaizer mechanisms, but to use Django logging mechanisms. If you do want to try and use standard DRF serializer mechanisms, you'd need to update DatabaseLogRecordSerializer to use a writeable nested serializer (to handle tags) - this is complex. So, I'd probably rename that class to something like DatabaseLogRecordReadOnlySerializer. And so something like:

import logging
logger = logging.getLogger("db")
def create(self, request, *args, **kwargs):
  for log_record_data in request.data:
    logger.info(
      <whatever the message you want to record is>,
      extra={"tags": [<whatever the tags you want to record are>]}
   )
  return Response(<not sure what should be returned; maybe serializations of the newly-added LogRecords?>)
marksmall commented 4 years ago

I'm not sure about using the Django Logging mechanism, I feel that is aimed at logging what happens at runtime, not tracking user movement, but I can see how it could be used. I don't know which is simpler, the DRF method seems more like what I would expect, but if it is a lot more complex, then I would go for the Django Logging method. So I am in a quandary, I don't know which to actually use, what is your opinion @allynt

allynt commented 3 years ago

This is a good question.

The original remit for this work - done as part of thermcert - was to add logging to our existing backend views. This meant that we didn't want the client to simply POST to the API and create an entry in the database. This worked outside of the client. Rather, we wanted to be able to redirect a logging message from anywhere in the codebase to the db. The current system does that:

logger = logging.getLogger("db")
logger.info("message")

can be added to an existing view, or a serializer, or some low-level method as needed. That feels like what was asked for.

But you're right that it doesn't do things the expected DRF way. I'm not sure I understand your point about tracking users vs logging runtime; user movement happens during runtime, does it not?

It's unclear what the best solution is.

However, it's worth noting that at some point, when we use something like logstash, we would probably need to hook into the logging framework anyway.

marksmall commented 3 years ago

Okay, so I am coming round to the django logger solution. So as I understand it, I still need to create the:

I'm not sure what we would do for GET requests, but the POSTing of data is our main focus. I'd expect the script @allynt has, not to use the API. I'm still not sure what benefit we would get from being able to view logs, but happy to do it if a use-case was presented.

@allynt am I on the right track or way off?

allynt commented 3 years ago

closed via #39