Skip to content

Conversation

@unique0lai
Copy link
Collaborator

No description provided.

@unique0lai unique0lai added feat A new feature. Correlates with MINOR in SemVer project/monitor project monitor labels Oct 24, 2025
@unique0lai unique0lai self-assigned this Oct 26, 2025
@unique0lai unique0lai requested a review from Copilot October 26, 2025 07:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR modifies the metadata query routing to use cluster-specific retention times instead of a global default. The change allows custom metric expiration times to be determined based on the associated VM cluster's configuration.

Key Changes:

  • Introduces cluster-specific retention time logic for filtering expired time series metrics
  • Refactors query optimization by replacing paginated filtering with direct filter() calls
  • Adds type annotations to improve type safety

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
bkmonitor/metadata/models/storage.py Added type annotation for default_settings field in ClusterInfo model
bkmonitor/metadata/models/space/space_table_id_redis.py Refactored _filter_ts_info to group metrics by cluster retention time; simplified query filtering logic
bkmonitor/metadata/models/space/space.py Added type annotation for dimension_values field in SpaceResource model
bkmonitor/metadata/models/space/constants.py Added DEFAULT_VM_RETENTION_TIME constant (60 days in seconds)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

BKCI_SYSTEM_TABLE_ID_PREFIX = "system."

# VM 集群默认过期时间, 单位为秒, 默认60天
DEFAULT_VM_RETENTION_TIME = 60 * 3600 * 24
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The calculation is incorrect. The comment states the default is 60 days, but the calculation 60 * 3600 * 24 equals 5,184,000 seconds which is 60 days. However, the comment says 'unit is seconds, default 60 days' which is ambiguous. If 60 represents days, the formula should be 60 * 24 * 3600. The current order is mathematically correct but unconventional. Consider reordering to 60 * 24 * 3600 for clarity (days * hours_per_day * seconds_per_hour).

Suggested change
DEFAULT_VM_RETENTION_TIME = 60 * 3600 * 24
DEFAULT_VM_RETENTION_TIME = 60 * 24 * 3600

Copilot uses AI. Check for mistakes.
cluster_type=models.ClusterInfo.TYPE_VM,
).only("cluster_id", "default_settings")
vm_cluster_id_retention_time_map: dict[int, int] = {
data.cluster_id: data.default_settings.get("retention_time") or DEFAULT_VM_RETENTION_TIME
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using or operator can incorrectly fall back to the default if retention_time is 0 or other falsy values. If a cluster intentionally sets retention_time to 0, this code will ignore that setting. Use if data.default_settings.get('retention_time') is not None else DEFAULT_VM_RETENTION_TIME instead.

Suggested change
data.cluster_id: data.default_settings.get("retention_time") or DEFAULT_VM_RETENTION_TIME
data.cluster_id: data.default_settings.get("retention_time") if data.default_settings.get("retention_time") is not None else DEFAULT_VM_RETENTION_TIME

Copilot uses AI. Check for mistakes.
@unique0lai unique0lai requested a review from Copilot October 27, 2025 02:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

for data in ts_group_fields:
group_id_field_map.setdefault(data["group_id"], set()).add(data["field_name"])
for retention_days, group_id_list in retention_time_group_ids_map.items():
# 计算该分组的 begin_time,使用集群的 retention_time(天数转换为秒数)
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states '天数转换为秒数' (converting days to seconds), but the code uses datetime.timedelta(days=retention_days) which takes days directly, not seconds. The comment should be updated to '使用集群的 retention_time(以天为单位)' (using cluster's retention_time in days).

Suggested change
# 计算该分组的 begin_time,使用集群的 retention_time(天数转换为秒数)
# 计算该分组的 begin_time,使用集群的 retention_time(以天为单位)

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat A new feature. Correlates with MINOR in SemVer project/monitor project monitor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant