[#12195] Add new agent list using uid #12249
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12249 +/- ##
============================================
- Coverage 33.79% 33.60% -0.19%
+ Complexity 10743 10740 -3
============================================
Files 3926 3941 +15
Lines 91287 91789 +502
Branches 9511 9547 +36
============================================
Hits 30848 30848
- Misses 57804 58307 +503
+ Partials 2635 2634 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
donghun-cho
force-pushed
the
uidAgentList
branch
2 times, most recently
from
0eb94cc to
1b15e95
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new agent list feature that leverages UID to uniquely identify agents across the system. The changes include:
- New data model classes (AgentListEntry, AgentListEntryAndStatus) and mappers for representing agent details.
- Enhancements to the AgentListService and corresponding HBase DAO implementations to support UID-based operations.
- Updates to the collector and API controller layers to integrate the new agent list into agent info insertion and retrieval.
Reviewed Changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/com/navercorp/pinpoint/web/vo/agent/AgentListEntryAndStatus.java | New POJO representing combined agent entry and its status. |
| web/src/main/java/com/navercorp/pinpoint/web/vo/agent/AgentListEntry.java | New POJO for agent list entries. |
| web/src/main/java/com/navercorp/pinpoint/web/service/AgentListServiceImpl.java | New service implementation for UID-based agent list operations. |
| web/src/main/java/com/navercorp/pinpoint/web/authorization/controller/UidAgentListController.java | New REST controller for UID agent list endpoints including sorting logic. |
| commons-server/src/main/java/com/navercorp/pinpoint/common/server/util/AgentListRowKeyUtils.java | Utility functions for constructing and parsing row keys based on UID and agent start time. |
| Collector modules | Adjustments to the agent info insertion in the collector and new HBase DAO in the collector for managing the UID agent list. |
Files not reviewed (3)
- collector/src/main/resources/pinpoint-collector-root.properties: Language not supported
- hbase/scripts/hbase-create-snappy.hbase: Language not supported
- hbase/scripts/hbase-create.hbase: Language not supported
Comments suppressed due to low confidence (1)
web/src/main/java/com/navercorp/pinpoint/web/service/AgentListServiceImpl.java:75
- [nitpick] Consider renaming the parameter 'agentListEntry' to 'agentListEntries' to better reflect that it is a collection of entries.
private AgentStatusQuery buildAgentStatusQuery(Collection<AgentListEntry> agentListEntry, long toTimestamp) {
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for listing agents based on UIDs, with new service methods, controller endpoints, and underlying HBase changes to manage agent names. Key changes include the implementation of new AgentListService methods and controllers for UID-based agent listing, the addition of new HBase table configurations (including splits) for better scalability, and updates to collector components for inserting UID agent list information.
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/com/navercorp/pinpoint/web/service/AgentListServiceImpl.java | Added methods to assemble agent lists using agent identifiers and statuses. |
| web/src/main/java/com/navercorp/pinpoint/web/service/AgentListService.java | Introduced new interface definitions for agent list services. |
| web/src/main/java/com/navercorp/pinpoint/web/authorization/controller/UidAgentListController.java | New REST endpoints to serve all and application-specific agent lists with optional range filtering and ordering. |
| web/src/main/java/com/navercorp/pinpoint/web/authorization/controller/UidAdminController.java | Provided admin endpoints for cleaning up inactive or inconsistent UIDs. |
| hbase/scripts/hbase-create(.hbase, -snappy.hbase) | Updated HBase table creation scripts with added table splits for ApplicationUid, ApplicationName, and AgentName. |
| commons-server/src/main/java/com/navercorp/pinpoint/common/server/uid/* | Introduced new mappers and utility classes for UID and agent identifier processing; updated HBaseCellData and ApplicationUid toString. |
| commons-hbase/src/main/java/com/navercorp/pinpoint/common/hbase/* | Added support for AGENT_NAME as a new HBase column family and table enum entry. |
| collector/src/main/java/com/navercorp/pinpoint/collector/* | Updated AgentInfoService and GRPC handler to integrate UID agent list insertion, including dependency injection modifications and error handling. |
Comments suppressed due to low confidence (1)
commons-server/src/main/java/com/navercorp/pinpoint/common/server/uid/ApplicationUid.java:50
- The toString method concatenates the uid with a unary '+' operator instead of a descriptive label. Replace "+ +uid" with "+ "uid=" + uid" to improve clarity.
return "ApplicationUid{" + +uid + '}'
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new agent list feature based on uid in both the web and collector modules while also updating related HBase table definitions and mappers. Key changes include new REST endpoints and controllers (UidAgentListController and UidAdminController), new HBase DAO and cell mapper implementations for agent name management, and modifications to the AgentInfoService to support uid-based operations.
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/com/navercorp/pinpoint/web/authorization/controller/UidAgentListController.java | New controller endpoints for fetching an agent list using uid. |
| web/src/main/java/com/navercorp/pinpoint/web/authorization/controller/UidAdminController.java | New admin endpoints for debugging and cleanup of uid-related agent data. |
| hbase/scripts/*.hbase | Updated HBase table definitions with additional split keys. |
| commons-server/src/main/java/com/navercorp/pinpoint/common/server/* | New and updated uid mappers, row key utils, and HBase cell data handling. |
| collector/src/main/java/com/navercorp/pinpoint/collector/* | Modifications in service and DAO layers to integrate uid-based agent list functionality. |
Comments suppressed due to low confidence (3)
collector/src/main/java/com/navercorp/pinpoint/collector/uid/UidModule.java:35
- The removal of the emptyApplicationUidService bean may affect components that previously relied on it. Confirm that this change is intentional and that there are no dependencies expecting that bean.
- @Bean
collector/src/main/java/com/navercorp/pinpoint/collector/service/AgentInfoService.java:59
- [nitpick] Consider renaming the constructor parameter 'agentListDao' to 'agentNameDao' for consistency with its usage and to improve clarity.
ApplicationUidService applicationUidService, Optional<AgentNameDao> agentListDao,
commons-server/src/main/java/com/navercorp/pinpoint/common/server/uid/HbaseCellData.java:11
- The constructor parameter order for HbaseCellData has changed (now accepting rowKey, timestamp, then valueObject), which may affect existing consumers. Please verify that all clients of this API are updated accordingly and consider updating the documentation.
public HbaseCellData(byte[] rowKey, long timestamp, Object valueObject) {
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for a new agent list feature using application UIDs. Key changes include the introduction of new REST endpoints in UidAgentListController and UidAdminController, updates to the HBase creation scripts with split definitions, and modifications in AgentInfoService for UID integration during agent insertion.
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/com/navercorp/pinpoint/web/authorization/controller/UidAgentListController.java | New REST endpoints to retrieve agent lists with optional range filters and ordering. |
| web/src/main/java/com/navercorp/pinpoint/web/authorization/controller/UidAdminController.java | New admin endpoints for agent and application UID debugging and cleanup. |
| hbase/scripts/hbase-create.hbase | Updated commented HBase table creation scripts with new split configurations. |
| hbase/scripts/hbase-create-snappy.hbase | Updated commented HBase table creation scripts with snappy compression and splits. |
| commons-server/src/main/java/com/navercorp/pinpoint/common/server/util/AgentListRowKeyUtils.java | Utility helper methods for constructing HBase row keys for agents. |
| (Other files) | Introduce/migrate supporting UID mapping, cell mapping, and change configurations to integrate the UID agent list feature. |
There was a problem hiding this comment.
Pull Request Overview
Adds support for an agent list indexed by UID and refactors cache configuration.
- Introduces new HBase mappers (
ApplicationUidCellMapper,ApplicationNameCellMapper,AgentNameMapper) and updatesHbaseTables/HbaseTableto includeAGENT_NAME. - Extends
AgentInfoServiceand gRPC handler to insert UID-based agent entries, and provides newHbaseAgentNameDao/AgentNameDao. - Replaces
CaffeineCacheSpecwithCaffeineCachePropertiesand inlines Caffeine builder methods in cache config classes.
Reviewed Changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| commons-server/.../uid/mapper/ApplicationUidCellMapper.java | New row mapper for ApplicationUid cells |
| commons-server/.../uid/mapper/ApplicationNameCellMapper.java | Renamed and adjusted ApplicationName cell mapper |
| commons-server/.../uid/mapper/AgentNameMapper.java | New mapper to extract agent name from HBase cells |
| commons-server/.../uid/HbaseCellData.java | Reordered constructor parameters for HbaseCellData |
| commons-hbase/.../HbaseTables.java & HbaseTable.java | Added AGENT_NAME table and column family |
| commons-server/.../cache/CaffeineCacheProperties.java | New properties class replacing old CaffeineCacheSpec |
| collector/.../config/ServiceUidCacheConfig.java & ApplicationUidConfig.java | Refactored to build Caffeine caches using CaffeineCacheProperties |
| collector/.../service/StaticServiceGroupService.java & EmptyApplicationUidService.java | Injected config values and added logging |
| collector/.../dao/hbase/HbaseAgentNameDao.java & dao/AgentNameDao.java | New DAO and interface for writing agent names to HBase |
| collector/.../service/AgentInfoService.java & handler/grpc/GrpcAgentInfoHandler.java | Updated to call new insert signature with service/application UIDs |
Comments suppressed due to low confidence (1)
commons-server/src/main/java/com/navercorp/pinpoint/common/server/uid/HbaseCellData.java:8
- The timestamp field has package-private visibility and is mutable; consider declaring it as private final to enforce immutability and encapsulation.
long timestamp;
| public void insert(Supplier<ServiceUid> serviceUidSupplier, Supplier<ApplicationUid> applicationUidSupplier, @Valid final AgentInfoBo agentInfoBo) { | ||
| agentInfoDao.insert(agentInfoBo); | ||
| applicationIndexDao.insert(agentInfoBo); | ||
| uidAgentListInsert(serviceUidSupplier, applicationUidSupplier, agentInfoBo); | ||
| } | ||
|
|
||
| private void uidAgentListInsert(Supplier<ServiceUid> serviceUidSupplier, Supplier<ApplicationUid> applicationUidSupplier, AgentInfoBo agentInfoBo) { | ||
| if (!uidAgentListEnable || agentNameDao == null) { | ||
| return; | ||
| } | ||
| ServiceUid serviceUid = serviceUidSupplier.get(); | ||
| ApplicationUid applicationUid = getApplicationUid(applicationUidSupplier, agentInfoBo, serviceUid); | ||
| insertApplicationName(agentInfoBo, serviceUid, applicationUid); | ||
| } | ||
|
|
||
| private ApplicationUid getApplicationUid(Supplier<ApplicationUid> applicationUidSupplier, AgentInfoBo agentInfoBo, ServiceUid serviceUid) { | ||
| try { | ||
| return applicationUidSupplier.get(); | ||
| } catch (UidException exception) { | ||
| return applicationUidService.getOrCreateApplicationUid(serviceUid, agentInfoBo.getApplicationName()); | ||
| } |
There was a problem hiding this comment.
The insert method signature now expects Supplier and Supplier, but the GRPC handler passes ServiceUid and ApplicationUid directly, causing a type mismatch. Consider changing the signature to accept ServiceUid and ApplicationUid directly or wrapping the parameters in Suppliers at the caller site.
| public void insert(Supplier<ServiceUid> serviceUidSupplier, Supplier<ApplicationUid> applicationUidSupplier, @Valid final AgentInfoBo agentInfoBo) { | |
| agentInfoDao.insert(agentInfoBo); | |
| applicationIndexDao.insert(agentInfoBo); | |
| uidAgentListInsert(serviceUidSupplier, applicationUidSupplier, agentInfoBo); | |
| } | |
| private void uidAgentListInsert(Supplier<ServiceUid> serviceUidSupplier, Supplier<ApplicationUid> applicationUidSupplier, AgentInfoBo agentInfoBo) { | |
| if (!uidAgentListEnable || agentNameDao == null) { | |
| return; | |
| } | |
| ServiceUid serviceUid = serviceUidSupplier.get(); | |
| ApplicationUid applicationUid = getApplicationUid(applicationUidSupplier, agentInfoBo, serviceUid); | |
| insertApplicationName(agentInfoBo, serviceUid, applicationUid); | |
| } | |
| private ApplicationUid getApplicationUid(Supplier<ApplicationUid> applicationUidSupplier, AgentInfoBo agentInfoBo, ServiceUid serviceUid) { | |
| try { | |
| return applicationUidSupplier.get(); | |
| } catch (UidException exception) { | |
| return applicationUidService.getOrCreateApplicationUid(serviceUid, agentInfoBo.getApplicationName()); | |
| } | |
| public void insert(ServiceUid serviceUid, ApplicationUid applicationUid, @Valid final AgentInfoBo agentInfoBo) { | |
| agentInfoDao.insert(agentInfoBo); | |
| applicationIndexDao.insert(agentInfoBo); | |
| uidAgentListInsert(serviceUid, applicationUid, agentInfoBo); | |
| } | |
| private void uidAgentListInsert(ServiceUid serviceUid, ApplicationUid applicationUid, AgentInfoBo agentInfoBo) { | |
| if (!uidAgentListEnable || agentNameDao == null) { | |
| return; | |
| } | |
| insertApplicationName(agentInfoBo, serviceUid, applicationUid); | |
| } | |
| private ApplicationUid getApplicationUid(ApplicationUid applicationUid, AgentInfoBo agentInfoBo, ServiceUid serviceUid) { | |
| if (applicationUid != null) { | |
| return applicationUid; | |
| } | |
| return applicationUidService.getOrCreateApplicationUid(serviceUid, agentInfoBo.getApplicationName()); |
Copilot uses AI. Check for mistakes.
| @@ -91,7 +91,7 @@ private PResult handleAgentInfo(ServerHeader header, PAgentInfo agentInfo) { | |||
| try { | |||
| // agent info | |||
| final AgentInfoBo agentInfoBo = this.agentInfoBoMapper.map(agentInfo, header); | |||
| this.agentInfoService.insert(agentInfoBo); | |||
| this.agentInfoService.insert(header.getServiceUid(), header.getApplicationUid(), agentInfoBo); | |||
There was a problem hiding this comment.
Calling insert with ServiceUid and ApplicationUid directly does not match the updated method signature expecting Suppliers. Wrap these calls in Supplier lambdas or update the service method signature.
| this.agentInfoService.insert(header.getServiceUid(), header.getApplicationUid(), agentInfoBo); | |
| this.agentInfoService.insert(() -> header.getServiceUid(), () -> header.getApplicationUid(), agentInfoBo); |
Copilot uses AI. Check for mistakes.
| return caffeineCacheManager; | ||
| } | ||
|
|
||
| private Caffeine<Object, Object> buildCaffeine(CaffeineCacheProperties properties) { |
There was a problem hiding this comment.
[nitpick] The buildCaffeine method is duplicated in both ServiceUidCacheConfig and ApplicationUidConfig; consider extracting it into a shared utility to reduce code duplication.
Copilot uses AI. Check for mistakes.
No description provided.