[DRAFT] feat: simplify telemetry #5441
Conversation
…ent),` instead of direct `_TELEMETRY_CLIENT` import
…ssend_telemetry-to-the-argilla-server
rafactor: add "me" to user operations refactor: add "list" to like-like operations
feat: add dataset size tracking
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/5204-feature-add-huggingface_hubutilssend_telemetry-to-the-argilla-server #5441 +/- ##
==================================================================================================================
+ Coverage 91.13% 91.30% +0.17%
==================================================================================================================
Files 139 139
Lines 5716 5774 +58
==================================================================================================================
+ Hits 5209 5272 +63
+ Misses 507 502 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if dataset: | ||
| for attr in attributes: | ||
| for attr in _RELEVANT_ATTRIBUTES + ["distribution"]: |
There was a problem hiding this comment.
The track_crud_dataset method is used for dataset creation. In that case, no fields, questions, or other setting relationships will be provided, so this code is not necessary. Also, the distribution is not a relationship but a column, so the is_relationship_loaded is not needed. You could track the distribution by accessing it directly. I think this simplifies the code readability
| for attr in _RELEVANT_ATTRIBUTES: | ||
| if dataset.is_relationship_loaded(attr): | ||
| obtained_attr_list = getattr(dataset, attr) | ||
| await self.track_resource_size( | ||
| crud_action=crud_action, setting_name=f"dataset/{attr}", count=len(obtained_attr_list) | ||
| ) | ||
|
|
There was a problem hiding this comment.
This info won't be available on dataset creation, so I would say to remove it.
| This method is used to track the creation, update, and deletion of dataset settings. These | ||
| settings include fields, questions, vectors settings, and metadata properties. | ||
|
|
||
| Args: |
There was a problem hiding this comment.
I think we use Parameters format instead.
| await telemetry_client.track_resource_size( | ||
| crud_action="read", setting_name="dataset", count=len(dataset_list) | ||
| ) |
There was a problem hiding this comment.
I think adding the dataset count when listing the datasets for the owner is a bit of a convoluted way of computing the metric. It might make more sense to send this information somewhere else in the system. I think I should rethink these metrics and include them in a second iteration.
There was a problem hiding this comment.
Also, not sure about the need for crud_action="read" here since is no related to a resource crud action (we compute a size)
| await telemetry_client.track_crud_user(action="list", user=None, is_oauth=False, count=len(users)) | ||
| for user in users: | ||
| await telemetry_client.track_crud_user(action="read", user=user, is_oauth=False) | ||
| await telemetry_client.track_resource_size(crud_action="read", setting_name="user", count=len(users)) |
There was a problem hiding this comment.
Similarly, computing this metric here can be not easy to understand later. Better if we reduce the number of metrics for the first iteration an think about these stats for a second iteration
| ): | ||
| await authorize(current_user, UserPolicy.list_workspaces) | ||
|
|
||
| user = await User.get_or_raise(db, user_id) | ||
|
|
||
| if user.is_owner: | ||
| workspaces = await accounts.list_workspaces(db) | ||
| await telemetry_client.track_resource_size(crud_action="read", setting_name="workspace", count=len(workspaces)) |
| return {"type": "vector"} | ||
| elif isinstance(setting, dict): | ||
| return {"type": f"distribution_{setting['strategy']}_{setting['min_submitted']}"} | ||
| raise NotImplementedError("Expected a setting to be processed.") |
There was a problem hiding this comment.
Having this here is a bit scary. We don't want to break endpoint functionally because the telemetry module cannot extract info. Also, in general, we must be sure to not propagate errors when telemetry info.
| if crud_action: | ||
| user_agent["request.endpoint.method"] = crud_action |
There was a problem hiding this comment.
what's the purpose of this attribute? We only are tracking "create" actions, and my previous PR is already tracking endpoint method info.
|
|
||
| async def track_data(self, topic: str, user_agent: dict, include_system_info: bool = True, count: int = 1): | ||
| library_name = "argilla" | ||
| async def track_data(self, topic: str, count: int = None, crud_action: str = None, user_agent: dict = None) -> None: |
There was a problem hiding this comment.
Since the track_data is the base method to send telemetry info, I would say to simplify the signature and move the count and the crud_action as part of the data. Also, the name of the user agent reveals implementation details of how the data is sent and is not aligned with the method name.
async def track_data (self, topic, data:dict)
...
| await self.track_resource_size( | ||
| crud_action=crud_action, setting_name=f"dataset/{attr}", count=len(obtained_attr_list) | ||
| ) | ||
|
|
||
| async def track_crud_dataset_setting( |
There was a problem hiding this comment.
Even if the telemetry library won't send data if disabled, we should prevent making unnecessary calls and data processing when telemetry is disabled. Also, if there is an error in our code, users have no way to disable the whole telemetry tracking.
# Description <!-- Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change. --> This PR adds the track startup method defined in #5441 and include perstitent_storaged_enbled info as part of the system info **Type of change** <!-- Please delete options that are not relevant. Remember to title the PR according to the type of change --> - Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** <!-- Please add some reference about how your feature has been tested. --> **Checklist** <!-- Please go over the list and make sure you've taken everything into account --> - I added relevant documentation - I followed the style guidelines of this project - I did a self-review of my code - I made corresponding changes to the documentation - I confirm My changes generate no new warnings - I have added tests that prove my fix is effective or that my feature works - I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)
Description
Closes NA