Skip to content

Conversation

@intr3p1d
Copy link
Contributor

No description provided.

@intr3p1d intr3p1d added this to the 3.1.0 milestone Dec 20, 2024
@intr3p1d intr3p1d self-assigned this Dec 20, 2024
@codecov
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 2.43902% with 680 lines in your changes missing coverage. Please review.

Project coverage is 33.35%. Comparing base (f003b8d) to head (0e4043e).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
...ctor/applicationmap/service/RedisTraceService.java 0.00% 83 Missing ⚠️
.../histogram/ApplicationMapNodeHistogramFactory.java 0.00% 74 Missing ⚠️
...t/collector/applicationmap/redis/RedisSelfDao.java 0.00% 44 Missing ⚠️
...ollector/applicationmap/redis/RedisInboundDao.java 0.00% 43 Missing ⚠️
...ionmap/dao/mapper/ApplicationMapInboundMapper.java 0.00% 43 Missing ⚠️
...onmap/dao/mapper/ApplicationMapOutboundMapper.java 0.00% 41 Missing ⚠️
...llector/applicationmap/redis/RedisOutboundDao.java 0.00% 37 Missing ⚠️
...p/dao/mapper/ApplicationMapResponseTimeMapper.java 0.00% 34 Missing ⚠️
...mon/server/applicationmap/ApplicationMapUtils.java 0.00% 32 Missing ⚠️
...tor/applicationmap/redis/schema/TimeSeriesKey.java 0.00% 28 Missing ⚠️
... and 27 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #11850      +/-   ##
============================================
- Coverage     33.59%   33.35%   -0.24%     
- Complexity    10617    10619       +2     
============================================
  Files          3870     3894      +24     
  Lines         91158    91848     +690     
  Branches       9620     9679      +59     
============================================
+ Hits          30621    30635      +14     
- Misses        57885    58557     +672     
- Partials       2652     2656       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@intr3p1d intr3p1d force-pushed the add_redis_applicationmap branch 2 times, most recently from c69d6cc to 711c711 Compare January 20, 2025 07:23
@intr3p1d intr3p1d force-pushed the add_redis_applicationmap branch 3 times, most recently from 514e981 to 7c356a2 Compare February 18, 2025 09:29
@intr3p1d intr3p1d force-pushed the add_redis_applicationmap branch 2 times, most recently from 707675d to a6c0db7 Compare February 27, 2025 09:00
@intr3p1d intr3p1d force-pushed the add_redis_applicationmap branch from a6c0db7 to 2c89b2e Compare March 6, 2025 08:27
@intr3p1d intr3p1d force-pushed the add_redis_applicationmap branch from 2c89b2e to 0e4043e Compare March 21, 2025 09:50
@emeroad emeroad requested a review from Copilot March 27, 2025 05:36
Copy link

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 introduces a new server map module that leverages Redis Timeseries for recording and aggregating application map statistics. Key changes include:

  • Addition of a new ApplicationMapModule along with corresponding Spring configuration to enable component scanning.
  • New Redis-based implementations for bulk writing and DAO interfaces (InboundDao, OutboundDao, SelfDao) to support bidirectional statistics updating.
  • Updates across multiple classes to build and manage time series keys and values for tracking application metrics.

Reviewed Changes

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

Show a summary per file
File Description
collector/src/main/java/com/navercorp/pinpoint/collector/applicationmap/ApplicationMapModule.java Introduces a new configuration module for the ApplicationMap feature.
collector/src/main/java/com/navercorp/pinpoint/collector/applicationmap/redis/schema/ApplicationMapTable.java Defines table types for categorizing Redis time series data.
collector/src/main/java/com/navercorp/pinpoint/collector/applicationmap/redis/statistics/RedisBulkFactory.java Creates RedisBulkWriter beans using a simple async Redis connection.
collector/src/main/java/com/navercorp/pinpoint/collector/applicationmap/redis/schema/TimeSeriesKey.java Constructs composite keys for Redis time series storage.
Other files Provide new DAO and service implementations across Redis self, inbound, and outbound operations, along with minor updates in BulkWriter and module registration.
Files not reviewed (1)
  • collector/pom.xml: Language not supported

this.subServiceId = new LabelToKey(TimeSeriesLabel.SUB_SERVICE_ID, String.valueOf(subServiceId));
this.subApplicationName = new LabelToKey(TimeSeriesLabel.SUB_APPLICATION_NAME, subApplicationName);
this.subServiceType = new LabelToKey(TimeSeriesLabel.SUB_SERVICE_TYPE_SLOT, String.valueOf(subServiceType));
this.slotNumber = new LabelToKey(TimeSeriesLabel.SUB_SERVICE_TYPE_SLOT, String.valueOf(slotNumber));
Copy link

Copilot AI Mar 27, 2025

Choose a reason for hiding this comment

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

Both subServiceType and slotNumber use the same TimeSeriesLabel.SUB_SERVICE_TYPE_SLOT, which may lead to key collisions or overwriting when converting to labels. Consider introducing a distinct label (e.g., SUB_SLOT_NUMBER) for slotNumber.

Suggested change
this.slotNumber = new LabelToKey(TimeSeriesLabel.SUB_SERVICE_TYPE_SLOT, String.valueOf(slotNumber));
this.slotNumber = new LabelToKey(TimeSeriesLabel.SUB_SLOT_NUMBER, String.valueOf(slotNumber));

Copilot uses AI. Check for mistakes.
Comment on lines 36 to 39

private RedisBulkWriter newRedisBulkWriter() {
RedisURI redisURI = RedisURI.create("redis://localhost:6379");
RedisClient client = RedisClient.create(redisURI);
Copy link

Copilot AI Mar 27, 2025

Choose a reason for hiding this comment

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

A new RedisClient is created without explicit lifecycle management or shutdown handling. Consider defining the client as a Spring bean with proper resource management to prevent potential resource leaks.

Suggested change
private RedisBulkWriter newRedisBulkWriter() {
RedisURI redisURI = RedisURI.create("redis://localhost:6379");
RedisClient client = RedisClient.create(redisURI);
@Bean
public RedisClient redisClient() {
RedisURI redisURI = RedisURI.create("redis://localhost:6379");
return RedisClient.create(redisURI);
}
private RedisBulkWriter newRedisBulkWriter(RedisClient client) {

Copilot uses AI. Check for mistakes.
@intr3p1d intr3p1d force-pushed the add_redis_applicationmap branch from 0e4043e to 9c952e5 Compare March 28, 2025 10:34
@intr3p1d intr3p1d force-pushed the add_redis_applicationmap branch from 9c952e5 to e4214be Compare April 3, 2025 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant