directus / v8-archive

Directus Database API β€” Wraps Custom SQL Databases with a REST/GraphQL API
https://docs.directus.io/api/reference.html
507 stars 204 forks source link

Add table for access logs #85

Closed rijkvanzanten closed 5 years ago

rijkvanzanten commented 6 years ago

Next to the activity in directus activity, the API needs a way to track the API access. Basically, this means a copy of directus_activity which only stores ENTRY-ACCESS or READ or something like that.

This needs more discussion. To be continued..

@benhaynes

benhaynes commented 6 years ago

Potential table names:

Values to store:

wellingguzman commented 6 years ago

I like directus_access because we log, access only.

I think the following values are out of access context:

rijkvanzanten commented 6 years ago

Server Latency would be the time it took from the moment the API received the request for data till the moment the server responds.

You can compare it to the thing Google does:

screen shot 2018-05-01 at 5 18 44 pm

(Would be nice if we have similar performance too πŸ˜„)

wellingguzman commented 6 years ago

I feel like this shouldn't be on access data. This would be useful to have some way or another.

rijkvanzanten commented 6 years ago

Could be an optional item in the meta object? πŸ™‚

wellingguzman commented 6 years ago

Oh, that's a good place to add this info too.

benhaynes commented 6 years ago

I like the idea of having the latency (if we can). I also think we should include errors (depending on what they are). If someone requests data and the database has to work (at all) then we need to track that as a request.

Think about Hosted... this table will be used so that we know how many API access hits there are per month... and errors should count towards that.

wellingguzman commented 6 years ago

Something to think is that this can be a done by hooks (as extension).

Accessing the API with or without error can count as access. Logging error are done in filesystem right now, saving error twice wouldn't be great idea.

I believe each have to be stored separately. Open to discuss it further.

benhaynes commented 6 years ago

Filesystem errors include EVERYTHING... all errors. It's an "error log". This has details of the problem/error and a log of what happened.

I just want a system store of ALL API access requests... it's an "access log". This would store "failed requests" not the actual error. This doesn't need to store any details about what went wrong (or right)... just that an attempt was made and the ip etc.

Therefore I see these two things as different.

rijkvanzanten commented 6 years ago

@WellingGuzman is right in the fact that this is pretty easy to build custom in a hook. No need to have it in core as a system table?

benhaynes commented 6 years ago

It may be easy to add as a custom extension, but then we don't actually have this as a feature – and I think it would be pretty useful for anyone. We can push it to 7.1 if it's a lot of work, but I think access logs are a pretty common/core feature with this type of platform/framework.

rijkvanzanten commented 6 years ago

Thought: it could also be a csv file

danielfrentz commented 6 years ago

Some thoughts: Add a configuration option to enable the logging, since it would add some processing time. Add an endpoint to get the logs. As a general rule people wont want the logs unless they are debugging something. Latency should be measured as the time the request is received and returned. This can be implemented as middle ware (and adding an attribute to the request).

danielfrentz commented 6 years ago

Additional thoughts: It might be a good idea to have a debug parameter for retrieving such information for a given request, rather than adding it to the meta data. for example: localhost/_/users?debug=true { "debug":{ "latency":"..." } //rest }

rijkvanzanten commented 6 years ago

Hey @danielfrentz, quick tip: you can use markdown formatting to make your code a little more readable on GitHub πŸ™‚

{ "debug":{ "latency":"..." } //rest }

{
    "debug": {
        "latency": "..."
    }
}

Just wrap the code in three backticks `

benhaynes commented 6 years ago

Definitely should be configurable (on/off at the very least). I'd say we should try to keep as much as possible in directus_settings so that it can be changed within the app interface.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

benhaynes commented 5 years ago

@rijkvanzanten – what are we thinking about this now that we're in Cloud dev?

rijkvanzanten commented 5 years ago

Cloud will handle this separately from Directus

benhaynes commented 5 years ago

Is it worth building this in a way so that it can be used as an extension by users of Core Directus? Or just keep it proprietary to Cloud?

rijkvanzanten commented 5 years ago

Extension will work just fine πŸ‘

It's basically going to be a hook for everything that will increment a counter on a separate instance. It's basically a web hook

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

benhaynes commented 5 years ago

To achieve better clarity/visibility, we are now tracking feature requests within the Feature Request project board.

This issue being closed does not mean it's not being considered.