Merge pull request #3062 from cpennington/comment-client-metrics

Add metrics to comment_client requests

common/static/coffee/src/discussion/utils.coffee

@@ -51,7 +51,6 @@ class @DiscussionUtil
       follow_discussion       : "/courses/#{$$course_id}/discussion/#{param}/follow"
       unfollow_discussion     : "/courses/#{$$course_id}/discussion/#{param}/unfollow"
       create_thread           : "/courses/#{$$course_id}/discussion/#{param}/threads/create"
-      search_similar_threads  : "/courses/#{$$course_id}/discussion/#{param}/threads/search_similar"
       update_thread           : "/courses/#{$$course_id}/discussion/threads/#{param}/update"
       create_comment          : "/courses/#{$$course_id}/discussion/threads/#{param}/reply"
       delete_thread           : "/courses/#{$$course_id}/discussion/threads/#{param}/delete"

lms/djangoapps/django_comment_client/base/urls.py

@@ -25,8 +25,6 @@ urlpatterns = patterns('django_comment_client.base.views',  # nopep8
     url(r'comments/(?P<comment_id>[\w\-]+)/flagAbuse$', 'flag_abuse_for_comment', name='flag_abuse_for_comment'),
     url(r'comments/(?P<comment_id>[\w\-]+)/unFlagAbuse$', 'un_flag_abuse_for_comment', name='un_flag_abuse_for_comment'),
     url(r'^(?P<commentable_id>[\w\-.]+)/threads/create$', 'create_thread', name='create_thread'),
-    # TODO should we search within the board?
-    url(r'^(?P<commentable_id>[\w\-.]+)/threads/search_similar$', 'search_similar_threads', name='search_similar_threads'),
     url(r'^(?P<commentable_id>[\w\-.]+)/follow$', 'follow_commentable', name='follow_commentable'),
     url(r'^(?P<commentable_id>[\w\-.]+)/unfollow$', 'unfollow_commentable', name='unfollow_commentable'),
 )

lms/djangoapps/django_comment_client/base/views.py

@@ -521,27 +521,6 @@ def unfollow_user(request, course_id, followed_user_id):
     return JsonResponse({})
 
 
-@require_GET
-def search_similar_threads(request, course_id, commentable_id):
-    """
-    given a course id and commentable id, run query given in text get param
-    of request
-    """
-    text = request.GET.get('text', None)
-    if text:
-        query_params = {
-            'text': text,
-            'commentable_id': commentable_id,
-        }
-        threads = cc.search_similar_threads(course_id, recursive=False, query_params=query_params)
-    else:
-        theads = []
-    context = {'threads': map(utils.extend_content, threads)}
-    return JsonResponse({
-        'html': render_to_string('discussion/_similar_posts.html', context)
-    })
-
-
 @require_POST
 @login_required
 @csrf.csrf_exempt

lms/djangoapps/django_comment_client/forum/tests.py

@@ -109,26 +109,18 @@ def make_mock_request_impl(text, thread_id=None):
     def mock_request_impl(*args, **kwargs):
         url = args[1]
         if url.endswith("threads"):
-            return Mock(
-                status_code=200,
-                text=json.dumps({
-                    "collection": [make_mock_thread_data(text, "dummy_thread_id", False)]
-                })
-            )
+            data = {
+                "collection": [make_mock_thread_data(text, "dummy_thread_id", False)]
+            }
         elif thread_id and url.endswith(thread_id):
-            return Mock(
-                status_code=200,
-                text=json.dumps(make_mock_thread_data(text, thread_id, True))
-            )
+            data = make_mock_thread_data(text, thread_id, True)
         else: # user query
-            return Mock(
-                status_code=200,
-                text=json.dumps({
-                    "upvoted_ids": [],
-                    "downvoted_ids": [],
-                    "subscribed_thread_ids": [],
-                })
-            )
+            data = {
+                "upvoted_ids": [],
+                "downvoted_ids": [],
+                "subscribed_thread_ids": [],
+            }
+        return Mock(status_code=200, text=json.dumps(data), json=Mock(return_value=data))
     return mock_request_impl
 
 

lms/djangoapps/django_comment_client/forum/views.py

@@ -196,12 +196,6 @@ def forum_form_discussion(request, course_id):
             'page': query_params['page'],
         })
     else:
-        #recent_active_threads = cc.search_recent_active_threads(
-        #    course_id,
-        #    recursive=False,
-        #    query_params={'follower_id': request.user.id},
-        #)
-
         with newrelic.agent.FunctionTrace(nr_transaction, "get_cohort_info"):
             cohorts = get_course_cohorts(course_id)
             cohorted_commentables = get_cohorted_commentables(course_id)
@@ -283,12 +277,6 @@ def single_thread(request, course_id, discussion_id, thread_id):
 
         threads = [utils.safe_content(thread) for thread in threads]
 
-        #recent_active_threads = cc.search_recent_active_threads(
-        #    course_id,
-        #    recursive=False,
-        #    query_params={'follower_id': request.user.id},
-        #)
-
         with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"):
             annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info)
 

lms/lib/comment_client/comment.py

@@ -21,6 +21,8 @@ class Comment(models.Model):
 
     initializable_fields = updatable_fields
 
+    metrics_tag_fields = ['course_id', 'endorsed', 'closed']
+
     base_url = "{prefix}/comments".format(prefix=settings.PREFIX)
     type = 'comment'
 
@@ -50,7 +52,13 @@ class Comment(models.Model):
         else:
             raise CommentClientRequestError("Can only flag/unflag threads or comments")
         params = {'user_id': user.id}
-        request = perform_request('put', url, params)
+        request = perform_request(
+            'put',
+            url,
+            params,
+            metric_tags=self._metric_tags,
+            metric_action='comment.abuse.flagged'
+        )
         voteable.update_attributes(request)
 
     def unFlagAbuse(self, user, voteable, removeAll):
@@ -65,7 +73,13 @@ class Comment(models.Model):
         if removeAll:
             params['all'] = True
 
-        request = perform_request('put', url, params)
+        request = perform_request(
+            'put',
+            url,
+            params,
+            metric_tags=self._metric_tags,
+            metric_action='comment.abuse.unflagged'
+        )
         voteable.update_attributes(request)
 
 

lms/lib/comment_client/comment_client.py

@@ -4,27 +4,3 @@ from .comment import Comment
 from .thread import Thread
 from .user import User
 from .commentable import Commentable
-
-from .utils import perform_request
-
-import settings
-
-
-def search_similar_threads(course_id, recursive=False, query_params={}, *args, **kwargs):
-    default_params = {'course_id': course_id, 'recursive': recursive}
-    attributes = dict(default_params.items() + query_params.items())
-    return perform_request('get', _url_for_search_similar_threads(), attributes, *args, **kwargs)
-
-
-def search_recent_active_threads(course_id, recursive=False, query_params={}, *args, **kwargs):
-    default_params = {'course_id': course_id, 'recursive': recursive}
-    attributes = dict(default_params.items() + query_params.items())
-    return perform_request('get', _url_for_search_recent_active_threads(), attributes, *args, **kwargs)
-
-
-def _url_for_search_similar_threads():
-    return "{prefix}/search/threads/more_like_this".format(prefix=settings.PREFIX)
-
-
-def _url_for_search_recent_active_threads():
-    return "{prefix}/search/threads/recent_active".format(prefix=settings.PREFIX)

lms/lib/comment_client/models.py

@@ -8,6 +8,7 @@ class Model(object):
     initializable_fields = ['id']
     base_url = None
     default_retrieve_params = {}
+    metric_tag_fields = []
 
     DEFAULT_ACTIONS_WITH_ID = ['get', 'put', 'delete']
     DEFAULT_ACTIONS_WITHOUT_ID = ['get_all', 'post']
@@ -62,9 +63,32 @@ class Model(object):
 
     def _retrieve(self, *args, **kwargs):
         url = self.url(action='get', params=self.attributes)
-        response = perform_request('get', url, self.default_retrieve_params)
+        response = perform_request(
+            'get',
+            url,
+            self.default_retrieve_params,
+            metric_tags=self._metric_tags,
+            metric_action='model.retrieve'
+        )
         self.update_attributes(**response)
 
+    @property
+    def _metric_tags(self):
+        """
+        Returns a list of tags to be used when recording metrics about this model.
+
+        Each field named in ``self.metric_tag_fields`` is used as a tag value,
+        under the key ``<class>.<metric_field>``. The tag model_class is used to
+        record the class name of the model.
+        """
+        tags = [
+            u'{}.{}:{}'.format(self.__class__.__name__, attr, self[attr])
+            for attr in self.metric_tag_fields
+            if attr in self.attributes
+        ]
+        tags.append(u'model_class:{}'.format(self.__class__.__name__))
+        return tags
+
     @classmethod
     def find(cls, id):
         return cls(id=id)
@@ -94,17 +118,29 @@ class Model(object):
         self.before_save(self)
         if self.id:   # if we have id already, treat this as an update
             url = self.url(action='put', params=self.attributes)
-            response = perform_request('put', url, self.updatable_attributes())
+            response = perform_request(
+                'put',
+                url,
+                self.updatable_attributes(),
+                metric_tags=self._metric_tags,
+                metric_action='model.update'
+            )
         else:   # otherwise, treat this as an insert
             url = self.url(action='post', params=self.attributes)
-            response = perform_request('post', url, self.initializable_attributes())
+            response = perform_request(
+                'post',
+                url,
+                self.initializable_attributes(),
+                metric_tags=self._metric_tags,
+                metric_action='model.insert'
+            )
         self.retrieved = True
         self.update_attributes(**response)
         self.after_save(self)
 
     def delete(self):
         url = self.url(action='delete', params=self.attributes)
-        response = perform_request('delete', url)
+        response = perform_request('delete', url, metric_tags=self._metric_tags, metric_action='model.delete')
         self.retrieved = True
         self.update_attributes(**response)
 

lms/lib/comment_client/thread.py

@@ -20,6 +20,11 @@ class Thread(models.Model):
         'closed', 'user_id', 'commentable_id', 'group_id', 'group_name', 'pinned'
     ]
 
+    metric_tag_fields = [
+        'course_id', 'group_id', 'pinned', 'closed', 'anonymous', 'anonymous_to_peers',
+        'endorsed', 'read'
+    ]
+
     initializable_fields = updatable_fields
 
     base_url = "{prefix}/threads".format(prefix=settings.PREFIX)
@@ -27,7 +32,7 @@ class Thread(models.Model):
     type = 'thread'
 
     @classmethod
-    def search(cls, query_params, *args, **kwargs):
+    def search(cls, query_params):
 
         default_params = {'page': 1,
                           'per_page': 20,
@@ -41,7 +46,14 @@ class Thread(models.Model):
             url = cls.url(action='get_all', params=extract(params, 'commentable_id'))
             if params.get('commentable_id'):
                 del params['commentable_id']
-        response = perform_request('get', url, params, *args, **kwargs)
+        response = perform_request(
+            'get',
+            url,
+            params,
+            metric_tags=[u'course_id:{}'.format(query_params['course_id'])],
+            metric_action='thread.search',
+            paged_results=True
+        )
         return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1)
 
     @classmethod
@@ -79,7 +91,13 @@ class Thread(models.Model):
         }
         request_params = strip_none(request_params)
 
-        response = perform_request('get', url, request_params)
+        response = perform_request(
+            'get',
+            url,
+            request_params,
+            metric_action='model.retrieve',
+            metric_tags=self._metric_tags
+        )
         self.update_attributes(**response)
 
     def flagAbuse(self, user, voteable):
@@ -90,7 +108,13 @@ class Thread(models.Model):
         else:
             raise CommentClientRequestError("Can only flag/unflag threads or comments")
         params = {'user_id': user.id}
-        request = perform_request('put', url, params)
+        request = perform_request(
+            'put',
+            url,
+            params,
+            metric_action='thread.abuse.flagged',
+            metric_tags=self._metric_tags
+        )
         voteable.update_attributes(request)
 
     def unFlagAbuse(self, user, voteable, removeAll):
@@ -105,19 +129,37 @@ class Thread(models.Model):
         if removeAll:
             params['all'] = True
 
-        request = perform_request('put', url, params)
+        request = perform_request(
+            'put',
+            url,
+            params,
+            metric_tags=self._metric_tags,
+            metric_action='thread.abuse.unflagged'
+        )
         voteable.update_attributes(request)
 
     def pin(self, user, thread_id):
         url = _url_for_pin_thread(thread_id)
         params = {'user_id': user.id}
-        request = perform_request('put', url, params)
+        request = perform_request(
+            'put',
+            url,
+            params,
+            metric_tags=self._metric_tags,
+            metric_action='thread.pin'
+        )
         self.update_attributes(request)
 
     def un_pin(self, user, thread_id):
         url = _url_for_un_pin_thread(thread_id)
         params = {'user_id': user.id}
-        request = perform_request('put', url, params)
+        request = perform_request(
+            'put',
+            url,
+            params,
+            metric_tags=self._metric_tags,
+            metric_action='thread.unpin'
+        )
         self.update_attributes(request)
 
 

lms/lib/comment_client/user.py

@@ -16,6 +16,8 @@ class User(models.Model):
     updatable_fields = ['username', 'external_id', 'email', 'default_sort_key']
     initializable_fields = updatable_fields
 
+    metric_tag_fields = ['course_id']
+
     base_url = "{prefix}/users".format(prefix=settings.PREFIX)
     default_retrieve_params = {'complete': True}
     type = 'user'
@@ -29,11 +31,23 @@ class User(models.Model):
 
     def follow(self, source):
         params = {'source_type': source.type, 'source_id': source.id}
-        response = perform_request('post', _url_for_subscription(self.id), params)
+        response = perform_request(
+            'post',
+            _url_for_subscription(self.id),
+            params,
+            metric_action='user.follow',
+            metric_tags=self._metric_tags + ['target.type:{}'.format(source.type)],
+        )
 
     def unfollow(self, source):
         params = {'source_type': source.type, 'source_id': source.id}
-        response = perform_request('delete', _url_for_subscription(self.id), params)
+        response = perform_request(
+            'delete',
+            _url_for_subscription(self.id),
+            params,
+            metric_action='user.unfollow',
+            metric_tags=self._metric_tags + ['target.type:{}'.format(source.type)],
+        )
 
     def vote(self, voteable, value):
         if voteable.type == 'thread':
@@ -43,7 +57,13 @@ class User(models.Model):
         else:
             raise CommentClientRequestError("Can only vote / unvote for threads or comments")
         params = {'user_id': self.id, 'value': value}
-        request = perform_request('put', url, params)
+        request = perform_request(
+            'put',
+            url,
+            params,
+            metric_action='user.vote',
+            metric_tags=self._metric_tags + ['target.type:{}'.format(voteable.type)],
+        )
         voteable.update_attributes(request)
 
     def unvote(self, voteable):
@@ -54,7 +74,13 @@ class User(models.Model):
         else:
             raise CommentClientRequestError("Can only vote / unvote for threads or comments")
         params = {'user_id': self.id}
-        request = perform_request('delete', url, params)
+        request = perform_request(
+            'delete',
+            url,
+            params,
+            metric_action='user.unvote',
+            metric_tags=self._metric_tags + ['target.type:{}'.format(voteable.type)],
+        )
         voteable.update_attributes(request)
 
     def active_threads(self, query_params={}):
@@ -63,7 +89,14 @@ class User(models.Model):
         url = _url_for_user_active_threads(self.id)
         params = {'course_id': self.course_id}
         params = merge_dict(params, query_params)
-        response = perform_request('get', url, params)
+        response = perform_request(
+            'get',
+            url,
+            params,
+            metric_action='user.active_threads',
+            metric_tags=self._metric_tags,
+            paged_results=True,
+        )
         return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1)
 
     def subscribed_threads(self, query_params={}):
@@ -72,7 +105,14 @@ class User(models.Model):
         url = _url_for_user_subscribed_threads(self.id)
         params = {'course_id': self.course_id}
         params = merge_dict(params, query_params)
-        response = perform_request('get', url, params)
+        response = perform_request(
+            'get',
+            url,
+            params,
+            metric_action='user.subscribed_threads',
+            metric_tags=self._metric_tags,
+            paged_results=True
+        )
         return response.get('collection', []), response.get('page', 1), response.get('num_pages', 1)
 
     def _retrieve(self, *args, **kwargs):
@@ -81,13 +121,25 @@ class User(models.Model):
         if self.attributes.get('course_id'):
             retrieve_params['course_id'] = self.course_id
         try:
-            response = perform_request('get', url, retrieve_params)
+            response = perform_request(
+                'get',
+                url,
+                retrieve_params,
+                metric_action='model.retrieve',
+                metric_tags=self._metric_tags,
+            )
         except CommentClientRequestError as e:
             if e.status_code == 404:
                 # attempt to gracefully recover from a previous failure
                 # to sync this user to the comments service.
                 self.save()
-                response = perform_request('get', url, retrieve_params)
+                response = perform_request(
+                    'get',
+                    url,
+                    retrieve_params,
+                    metric_action='model.retrieve',
+                    metric_tags=self._metric_tags,
+                )
             else:
                 raise
         self.update_attributes(**response)

lms/lib/comment_client/utils.py

@@ -33,12 +33,13 @@ def merge_dict(dic1, dic2):
 
 
 @contextmanager
-def request_timer(request_id, method, url):
+def request_timer(request_id, method, url, tags=None):
     start = time()
-    yield
+    with dog_stats_api.timer('comment_client.request.time', tags=tags):
+        yield
     end = time()
     duration = end - start
-    dog_stats_api.histogram('comment_client.request.time', duration, end)
+
     log.info(
         "comment_client_request_log: request_id={request_id}, method={method}, "
         "url={url}, duration={duration}".format(
@@ -50,7 +51,16 @@ def request_timer(request_id, method, url):
     )
 
 
-def perform_request(method, url, data_or_params=None, *args, **kwargs):
+def perform_request(method, url, data_or_params=None, raw=False,
+                    metric_action=None, metric_tags=None, paged_results=False):
+
+    if metric_tags is None:
+        metric_tags = []
+
+    metric_tags.append(u'method:{}'.format(method))
+    if metric_action:
+        metric_tags.append(u'action:{}'.format(metric_action))
+
     if data_or_params is None:
         data_or_params = {}
     headers = {
@@ -66,7 +76,7 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs):
     else:
         data = None
         params = merge_dict(data_or_params, request_id_dict)
-    with request_timer(request_id, method, url):
+    with request_timer(request_id, method, url, metric_tags):
         response = requests.request(
             method,
             url,
@@ -76,6 +86,14 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs):
             timeout=5
         )
 
+    metric_tags.append(u'status_code:{}'.format(response.status_code))
+    if response.status_code > 200:
+        metric_tags.append(u'result:failure')
+    else:
+        metric_tags.append(u'result:success')
+
+    dog_stats_api.increment('comment_client.request.count', tags=metric_tags)
+
     if 200 < response.status_code < 500:
         raise CommentClientRequestError(response.text, response.status_code)
     # Heroku returns a 503 when an application is in maintenance mode
@@ -84,10 +102,27 @@ def perform_request(method, url, data_or_params=None, *args, **kwargs):
     elif response.status_code == 500:
         raise CommentClient500Error(response.text)
     else:
-        if kwargs.get("raw", False):
+        if raw:
             return response.text
         else:
-            return json.loads(response.text)
+            data = response.json()
+            if paged_results:
+                dog_stats_api.histogram(
+                    'comment_client.request.paged.result_count',
+                    value=len(data.get('collection', [])),
+                    tags=metric_tags
+                )
+                dog_stats_api.histogram(
+                    'comment_client.request.paged.page',
+                    value=data.get('page', 1),
+                    tags=metric_tags
+                )
+                dog_stats_api.histogram(
+                    'comment_client.request.paged.num_pages',
+                    value=data.get('num_pages', 1),
+                    tags=metric_tags
+                )
+            return data
 
 
 class CommentClientError(Exception):