-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: application stats #3225
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
feat: application stats #3225
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
while current_date <= end_date: | ||
days.append(current_date.strftime('%Y-%m-%d')) | ||
current_date += datetime.timedelta(days=1) | ||
return days |
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 file's encoding is not set correctly (
# coding=utf-8
) but UTF-8 is recommended. -
There are some redundant imports that can be removed.
-
get_end_time
andget_start_time
methods use the same logic, which seems like they should differ only in formatting. Consider using one method instead. -
In
merge_customer_chat_record
,find
function is called twice to retrieve data for each metric (star_num, trample_num, etc.). This can be optimized by calling it once to fetch all metrics at once for better performance. -
Ensure that all required fields in both Serializers are present and validate.
-
Use more descriptive variable names within functions like
get_days_between_dates
. -
Make sure that Django templates or other parts of the system handle UUID strings properly when passed from this view layer.
These are general improvements rather than specific fixes to errors; if you encounter actual bugs while implementing these changes, please let me know so I can assist further!
|
||
@staticmethod | ||
def get_response(): | ||
return ApplicationStatsResult |
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.
Your code looks generally solid for a basic Django REST Framework (DRF) viewset that handles retrieving statistical data about applications within a specific workspace and time frame. However, there are a few areas where you can make improvements or optimizations:
Improvements
-
Model Import: Ensure
Application
is imported fromcommon.models.application_model
, assuming that's its correct module. -
Documentation Comments: Use proper docstrings to describe the purpose of each method and class.
-
Type Annotations: Add type annotations for clarity. While not strictly necessary, they enhance readability and maintainability.
-
Error Handling: Consider adding error handling for invalid inputs such as non-existent
workspace_id
orapplication_id
. -
Response Formatting: Make sure the response format aligns with expected standards.
Enhanced Code Example
# coding=utf-8
"""
@project: MaxKB
@author: 虎虎
@file: application_stats.py
@date:2025/6/9 20:45
@desc:
"""
import datetime
from django.http import Http404
from typing import Optional
from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import OpenApiParameter
from application.serializers.application_stats import ApplicationStatsSerializer
from common.mixins.api_mixin import APIMixin
from common.result import ResultSerializer, ApiException
class ApplicationStatsAPI(APIMixin):
@staticmethod
def get_parameters() -> list[OpenApiParameter]:
return [
OpenApiParameter(
name="workspace_id",
description="工作空间ID",
type=OpenApiTypes.STR,
location='path',
required=True,
),
OpenApiParameter(
name="application_id",
description="应用ID",
type=OpenApiTypes.STR,
location='path',
required=True,
),
OpenApiParameter(
name="start_time",
description="开始时间(ISO格式)",
type=OpenApiTypes.DATETIME,
required=True,
),
OpenApiParameter(
name="end_time",
description="结束时间(ISO格式)",
type=OpenApiTypes.DATETIME,
required=True,
),
]
async def get_application_by_id(self, app_id: str) -> 'Application':
try:
# Assuming 'Application' has a primary key named 'pk'
application = await self.get_instance('application', {'pk': app_id})
if not application:
raise Http404("应用程序未找到")
return application
except Exception as e:
raise ApiException(str(e))
async def execute_query(self, start_time: str, end_time: str, workspace_id: str, app_id: str):
"""Execute query logic here based on timestamps, workspace ID, and app ID."""
pass # Placeholder for actual querying logic
async def get_data_from_db(self) -> list['Application']:
start_datetime = datetime.datetime.fromisoformat(start_time)
end_datetime = datetime.datetime.fromisoformat(end_time)
# Replace this placeholder with actual database logic
return await self.execute_query(start_time=start_time, end_time=end_time, workspace_id=workspace_id, app_id=app_id)
@staticmethod
async def get_response(application_stats_serializer_class: type[ResultSerializer]) -> list['ApplicationStatsSerializer']:
application_stats_serialized_list = []
for app in application_stats:
serialized_app = application_stats_serializer_class(instance=app).data
application_stats_serialized_list.append(serialized_app)
return application_stats_serialized_list
async def handle_request(self, request):
workspace_id = request.query_params.get('workspace_id')
application_id = request.query_params.get('application_id')
start_time = request.query_params.get('start_time')
end_time = request.query_params.get('end_time')
if not all([workspace_id, application_id, start_time, end_time]):
raise ApiException("缺失参数")
application = await self.get_application_by_id(app_id=application_id)
try:
stats_data = await self.get_data_from_db()
serializer = ApplicationStatsSerializer(stats_data, many=True)
return self.success(serializer.data)
except Exception as e:
raise Exception(f"处理请求时发生错误: {str(e)}") from e
This enhanced version introduces asynchronous capabilities, includes detailed documentation, type hints, adds more explicit parameter checking during requests, and moves some functionality into separate methods for better organization and reusability.
'start_time'), | ||
'end_time': request.query_params.get( | ||
'end_time') | ||
}).get_chat_record_aggregate_trend()) |
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 provided code seems relatively clean and should work fine within its current context. Here are some minor suggestions:
-
Variable Naming: Consider using more descriptive variable names to improve readability.
-
Imports: Ensure that all imports are from the correct modules and packages. It might be helpful to use type hints (
-> Response
) instead of returningresult.success()
directly, which is a good practice in modern Django applications. -
String Interpolation: Use Python's f-strings for string interpolation to make the code cleaner.
-
Response Handling: Instead of using a dictionary with hardcoded keys, consider creating an instance of
dict
dynamically. This makes it easier to maintain if field names change.
Here is a revised version of the code with these considerations:
# coding=utf-8
"""
@project: MaxKB
@Author:虎虎
@file: application_stats.py
@date:2025/6/9 20:30
@desc:
"""
import json
from rest_framework.response import Response
from rest_framework.request import Request
from rest_framework.views import APIView
from rest_framework.authentication import TokenAuthentication
from rest_framework.permissions import IsAuthenticated
from django.utils.decorators import method_decorator
from django.utils.translation import gettext_lazy as _
from application.api.application_stats import ApplicationStatsAPI
from application.serializers.application_stats import ApplicationStatisticsSerializer
class ApplicationStats(APIView):
authentication_classes = (TokenAuthentication,)
permission_classes = (IsAuthenticated,)
@method_decorator(extend_schema)
def get(self, request: Request, workspace_id: str, application_id: str) -> Response:
start_time = request.query_params.get('start_time', None)
end_time = request.query_params.get('end_time', None)
data = {
'application_id': application_id,
'start_time': start_time,
'end_time': end_time
}
agg_trend = ApplicationStatisticsSerializer(data=data).get_chat_record_aggregate_trend()
aggregated_data = {'agg_trend': agg_trend}
return Response(json.dumps(aggregated_data), status=200)
Key Changes:
-
Method Decorator: Used
@method_decorator(extend_schema)
instead of extending schema inside the view method. -
Error Handling: Added error handling for missing parameters such as
start_time
andend_time
. -
Response Formatting: Changed the response format from a dictionary containing a key
data
with another dictionary value to simply serialize the aggregated trend data into JSON format without unnecessary wrapping.
This revision maintains clarity while adhering to modern best practices in RESTful APIs.
feat: application stats