Skip to content

Conversation

@1StephenCurry1
Copy link
Collaborator

No description provided.

@1StephenCurry1 1StephenCurry1 added feat A new feature. Correlates with MINOR in SemVer project/monitor project monitor labels Sep 2, 2025
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 tenant ID default value logic in time_series.py and ds_rt.py files by standardizing the handling of tenant IDs across multiple modules. The changes ensure proper tenant ID management in multi-tenant environments by requiring explicit tenant ID parameters instead of using default values internally.

Key changes:

  • Modified function signatures to require explicit bk_tenant_id parameters
  • Updated test files to pass tenant ID parameters explicitly
  • Added proper tenant ID retrieval logic using space_uid mapping functions

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
bkmonitor/metadata/tests/space/test_space_router.py Updated test to pass DEFAULT_TENANT_ID as bk_tenant_id parameter
bkmonitor/metadata/tests/space/test_compose_space_to_result_table_data.py Modified test calls to include DEFAULT_TENANT_ID parameter
bkmonitor/metadata/task/tasks.py Added tenant ID retrieval logic using space_uid mapping
bkmonitor/metadata/task/sync_space.py Implemented tenant-based grouping for space router operations
bkmonitor/metadata/resources/vm.py Updated RequestSerializer to include bk_tenant_id field
bkmonitor/metadata/resources/resources.py Standardized tenant ID handling across multiple resource classes
bkmonitor/metadata/models/storage.py Updated function call to include bk_tenant_id parameter
bkmonitor/metadata/models/space/space_table_id_redis.py Modified method signatures to require explicit bk_tenant_id parameters
bkmonitor/metadata/models/space/ds_rt.py Updated function signatures to make bk_tenant_id a required parameter
bkmonitor/metadata/models/record_rule/rules.py Added tenant ID retrieval logic for space operations
bkmonitor/metadata/management/commands/access_bkdata_vm.py Added tenant ID retrieval using space_uid mapping
bkmonitor/bkmonitor/utils/tenant.py Added new utility function for tenant ID conversion
bkmonitor/apm/models/datasource.py Updated APM models to use proper tenant ID retrieval
bkmonitor/apm/core/handlers/bk_data/virtual_metric.py Modified to use tenant ID conversion functions
bkmonitor/apm/core/handlers/bk_data/flow.py Updated flow handling to use proper tenant ID retrieval
bkmonitor/apm/core/discover/precalculation/storage.py Added tenant ID parameters for storage operations
bkmonitor/apm/core/discover/precalculation/consul_handler.py Updated to use application tenant ID

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 708 to 712
bk_tenant_id: str | None = DEFAULT_TENANT_ID,
space_type: str | None = None,
space_id: str | None = None,
space_id_list: list[str] | None = None,
is_publish: bool | None = True,
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

Moving bk_tenant_id to the first parameter position changes the function signature. This is a breaking change that could affect existing callers. Consider maintaining backward compatibility by keeping the original parameter order or providing a deprecation period.

Suggested change
bk_tenant_id: str | None = DEFAULT_TENANT_ID,
space_type: str | None = None,
space_id: str | None = None,
space_id_list: list[str] | None = None,
is_publish: bool | None = True,
space_type: str | None = None,
space_id: str | None = None,
space_id_list: list[str] | None = None,
is_publish: bool | None = True,
bk_tenant_id: str | None = DEFAULT_TENANT_ID,

Copilot uses AI. Check for mistakes.
Comment on lines +780 to +781
tid_ds = get_space_table_id_data_id(
space_type=space["space_type"], space_id=space["space_id"], bk_tenant_id=DEFAULT_TENANT_ID
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

Using DEFAULT_TENANT_ID instead of the actual tenant ID space['bk_tenant_id'] defeats the purpose of multi-tenant support. This should use the space's actual tenant ID.

Suggested change
tid_ds = get_space_table_id_data_id(
space_type=space["space_type"], space_id=space["space_id"], bk_tenant_id=DEFAULT_TENANT_ID
space_type=space["space_type"], space_id=space["space_id"], bk_tenant_id=space["bk_tenant_id"]

Copilot uses AI. Check for mistakes.
Comment on lines +787 to +792
space_client.push_table_id_detail(
table_id_list=table_id_list,
is_publish=is_publish,
include_es_table_ids=True,
bk_tenant_id=DEFAULT_TENANT_ID,
)
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

Using DEFAULT_TENANT_ID hardcoded value contradicts the multi-tenant logic implemented elsewhere in this function. This should use the appropriate tenant ID from the space context.

Copilot uses AI. Check for mistakes.
def get_cluster_data_ids(
cluster_id_list: list, table_id_list: list | None = None, bk_tenant_id: str | None = DEFAULT_TENANT_ID
) -> dict:
def get_cluster_data_ids(cluster_id_list: list, bk_tenant_id: str, table_id_list: list | None = None) -> dict:
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

The parameter order change (moving bk_tenant_id to the second position) is a breaking change. Consider maintaining backward compatibility or documenting this as a breaking change.

Suggested change
def get_cluster_data_ids(cluster_id_list: list, bk_tenant_id: str, table_id_list: list | None = None) -> dict:
def get_cluster_data_ids(bk_tenant_id: str, cluster_id_list: list, table_id_list: list | None = None) -> dict:

Copilot uses AI. Check for mistakes.
Comment on lines +200 to +203
apm_flow_ins = ApmFlow(
bk_biz_id=self.bk_biz_id, app_name=self.app_name, data_id=self.metric_datasource.bk_data_id, config=None
)
params = apm_flow_ins.get_deploy_params(
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

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

Creating an ApmFlow instance just to call get_deploy_params is inefficient. Since get_deploy_params was changed from a class method to an instance method, consider if this instantiation is necessary or if the method should remain as a class method.

Suggested change
apm_flow_ins = ApmFlow(
bk_biz_id=self.bk_biz_id, app_name=self.app_name, data_id=self.metric_datasource.bk_data_id, config=None
)
params = apm_flow_ins.get_deploy_params(
params = ApmFlow.get_deploy_params(

Copilot uses AI. Check for mistakes.
…6984615' of github.com:1StephenCurry1/bk-monitor into feat/metadata-time_series-ds_rt-tenant_id/#1010158081126984615
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