-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
WIP: move events serverside #2661
Conversation
addresses getredash#1501
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.
Thanks! See my comments. It's mainly about which object_type/id/etc to use.
redash/handlers/alerts.py
Outdated
@@ -14,6 +14,12 @@ class AlertResource(BaseResource): | |||
def get(self, alert_id): | |||
alert = get_object_or_404(models.Alert.get_by_id_and_org, alert_id, self.current_org) | |||
require_access(alert.groups, self.current_user, view_only) | |||
self.record_event({ | |||
'action': 'view', | |||
'timestamp': int(time.time()), |
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.
timestamp
is being added by self.record_event
,so no need to pass it manually (of course this applies in all other similar instances).
redash/handlers/data_sources.py
Outdated
self.record_event({ | ||
'action': 'view', | ||
'object_id': page, | ||
'object_type': 'api_call', |
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.
Here the object_type
should be datasource
and object_id
should be ds.id
.
redash/handlers/queries.py
Outdated
@@ -49,6 +49,12 @@ def get(self): | |||
|
|||
include_drafts = request.args.get('include_drafts') is not None | |||
|
|||
self.record_event({ | |||
'action': 'search', | |||
'object_id': term, |
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.
We should pass term as its own field, i.e.:
self.record_event({'action': 'search', 'object_type': 'query', 'term': term})
redash/handlers/admin.py
Outdated
'object_type': 'api_call', | ||
'object_id': 'admin/outdated_queries', | ||
'timestamp': int(time.time()), | ||
}) |
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.
action
-> list
object_type
-> outdated_queries
(object_id
can be null and can be ommited)
redash/handlers/admin.py
Outdated
'object_type': 'api_call', | ||
'object_id': 'admin/tasks', | ||
'timestamp': int(time.time()), | ||
}) |
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.
action
-> list
object_type
-> celery_tasks
redash/handlers/alerts.py
Outdated
'timestamp': int(time.time()), | ||
'object_id': 'alerts', | ||
'object_type': 'api_call' | ||
}) |
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.
Let's convert all the list actions, where you used object_type
=api_call
to:
action
-> list
object_type
-> <the relevant resource type>
(in this case it will be alert
, to be consistent with the other events for this type).
pull upstream to alison's fork july 4
@arikfr Updated. :) |
@alison985 This needs another pass to rebase things sadly. |
pull upstream to fork 7.22.18
addresses getredash#1501
This pull request is automatically deployed with Now. To access deployments, click Details below or on the icon next to each push. |
@jezdez rebased |
pull from upstream 7.29.18
pull merge 6 of upstream to branch
pull upstream to my fork
@alison985 We can safely ignore the Code Climate issues for now. (especially until #2727 has received some feedback) |
Needs another rebase 🤓 |
redash/handlers/data_sources.py
Outdated
self.record_event({ | ||
'action': 'list', | ||
'object_id': 'admin/data_sources', | ||
'object_type': 'data_sources', |
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.
The object type for this event is datasource
below, but data_sources
here. Should this be the same?
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.
Thanks for catching these!
redash/handlers/alerts.py
Outdated
self.record_event({ | ||
'action': 'list', | ||
'object_id': 'alerts', | ||
'object_type': 'alerts' |
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.
The object type is different to what is used above. Should this be the same?
- Use new master / rc release release strategy (getredash#440) - Migrate Circle CI 2.0 (getredash#488, getredash#502)
…ash#336) * Support authentication for URL data source (re getredash#330) * Refactor authentication support for data sources. Adds a new BaseHTTPQueryRunner class.
In the long run we'll be able to install additional dependencies by having an own Dockerfile to build images based on the Redash image but that installs additional Python dependencies. But until we have a fork with lots of changes ourselves we need to do it this way. Redash-stmo contains the ability to hook up our own Dockerflow library. Refs #13 Refs getredash#37
…dash#311) Extend the Remote User Auth backend with the ability to pass remote user groups via a configurable request header similar to the REMOTE_USER header. Refs getredash#37. If enabled the feature allows checks the header value against a configured list of group names, including the ability to use UNIX shell-style wildcards.
* Use --max-tasks-per-child as per celery documentation * Set --max-memory-per-child to 1/4th of total system memory * Split exec command over multiple lines * Fix memory variable typo
Merge upstream to my fork's master
addresses getredash#1501
addresses getredash#1501
… into alison_1501
@jezdez I made changes based on your comments, rebased again, and address the CodeClimate issues that I deemed reasonable and were in files touched by this PR. Now there are a TON of other files changed in this PR that I didn't touch. I'm adding WIP to the title of the PR so it doesn't get accidentally merged and will send you a meeting request to discuss. |
relates to #1501 (though I don't know if it should close it)
This is a port of a change in the Mozilla fork. It was previously reviewed by Arik here: mozilla#246
It does not port the move of the query version events because that feature isn't in upstream.