[#noissue] Add Separate api to get response histogram#12489
[#noissue] Add Separate api to get response histogram#12489intr3p1d merged 1 commit intopinpoint-apm:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a separate API to obtain response histograms by adding a new HistogramService and its implementation, along with updating the controller and configuration to integrate the new service.
- Added HistogramService interface and HistogramServiceImpl for handling application map selection and extraction of application endpoints.
- Extended MapHistogramController with a new endpoint (getOnlyResponseTimeHistogramDataV2) and updated dependency injection in ApplicationMapModule.
- Introduced a getter in LinkDataMap to support the new usage.
Reviewed Changes
Copilot reviewed 5 out of 5 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/applicationmap/service/HistogramServiceImpl.java | New implementation for application map histogram selection. |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/HistogramService.java | New service interface to support histogram API calls. |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/rawdata/LinkDataMap.java | Added getter for accessing linkDataMap. |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/controller/MapHistogramController.java | New endpoint added and necessary updates to support histogram API. |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/config/ApplicationMapModule.java | Updated dependency injection for the histogram controller. |
Comments suppressed due to low confidence (1)
web/src/main/java/com/navercorp/pinpoint/web/applicationmap/controller/MapHistogramController.java:101
- The null-check error message for applicationValidator incorrectly uses 'applicationFactory'. This should be updated to 'applicationValidator' for clarity.
this.applicationValidator = Objects.requireNonNull(applicationValidator, "applicationFactory");
intr3p1d
force-pushed
the
add_histogram_api
branch
from
e9d4af6 to
b75ad8a
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds a standalone API to fetch response-time histograms alongside the existing map data by introducing a new service and controller endpoint.
- Introduces
HistogramServiceinterface and its implementation to select application-link histograms. - Exposes internal link data via a new getter in
LinkDataMap. - Adds a
/getOnlyResponseTimeHistogramDataV2endpoint inMapHistogramControllerand wiresHistogramServicein the module.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/HistogramService.java | New interface defining histogram-specific methods |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/HistogramServiceImpl.java | Implementation of the histogram service |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/rawdata/LinkDataMap.java | Added getLinkDataMap() to expose the internal map |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/controller/MapHistogramController.java | Added new endpoint /getOnlyResponseTimeHistogramDataV2 and depth form |
| web/src/main/java/com/navercorp/pinpoint/web/applicationmap/config/ApplicationMapModule.java | Injected HistogramService into the controller bean definition |
Comments suppressed due to low confidence (3)
web/src/main/java/com/navercorp/pinpoint/web/applicationmap/controller/MapHistogramController.java:210
- The method name matches the existing endpoint rather than the new
/getOnlyResponseTimeHistogramDataV2mapping; rename it togetOnlyResponseTimeHistogramDataV2to avoid confusion.
public NodeHistogramSummaryView getResponseTimeHistogramDataV2(
web/src/main/java/com/navercorp/pinpoint/web/applicationmap/service/HistogramServiceImpl.java:37
- No unit or integration tests accompany this new service and its controller endpoint; add tests to validate key behaviors and guard against regressions.
/*
web/src/main/java/com/navercorp/pinpoint/web/applicationmap/controller/MapHistogramController.java:216
- Depth parameters are bound from the form but no explicit field constraints are shown; ensure
SearchDepthFormhas validation annotations to prevent out-of-range values.
@Valid @ModelAttribute SearchDepthForm depthForm,
| } | ||
|
|
||
| public Map<LinkKey, LinkData> getLinkDataMap() { | ||
| return linkDataMap; |
There was a problem hiding this comment.
Returning the internal mutable map exposes implementation details and allows external mutation; consider wrapping with Collections.unmodifiableMap(linkDataMap) to preserve encapsulation.
| return linkDataMap; | |
| return Collections.unmodifiableMap(linkDataMap); |
|
|
||
| TimeWindow timeWindow = option.getTimeWindow(); | ||
| LinkDataDuplexMap linkDataDuplexMap = linkSelector.select(Collections.singletonList(option.getSourceApplication()), timeWindow, outSearchDepth, inSearchDepth, timeAggregate); | ||
| watch.stop(); |
There was a problem hiding this comment.
[nitpick] You stop the stopwatch but never log its elapsed time; consider logging watch.getTotalTimeMillis() or using watch.prettyPrint() for better observability.
| watch.stop(); | |
| watch.stop(); | |
| logger.debug("Elapsed time for ApplicationMap Link Fetch: {} ms", watch.getTotalTimeMillis()); |
| return new NodeHistogramSummaryView(nodeHistogramSummary, serverGroupListView, format); | ||
| } | ||
|
|
||
| @GetMapping(value = "/getOnlyResponseTimeHistogramDataV2") |
There was a problem hiding this comment.
[nitpick] The path includes an HTTP verb and version in camelCase; consider using RESTful, resource-oriented naming (e.g., /v2/histograms/response-time/only).
| @GetMapping(value = "/getOnlyResponseTimeHistogramDataV2") | |
| @GetMapping(value = "/v2/histograms/response-time/only") |
| * @author intr3p1d | ||
| */ | ||
| public interface HistogramService { | ||
| LinkDataDuplexMap selectApplicationMap(MapServiceOption option); | ||
| List<Application> getFromApplications(LinkDataDuplexMap linkDataDuplexMap); |
There was a problem hiding this comment.
[nitpick] The new service interface lacks Javadoc; please add class- and method-level comments to explain its responsibility and usage.
| * @author intr3p1d | |
| */ | |
| public interface HistogramService { | |
| LinkDataDuplexMap selectApplicationMap(MapServiceOption option); | |
| List<Application> getFromApplications(LinkDataDuplexMap linkDataDuplexMap); | |
| * The {@code HistogramService} interface provides methods for retrieving and processing | |
| * application map data, including histograms and related application information. | |
| * <p> | |
| * Implementations of this interface are responsible for querying and transforming | |
| * data into a format suitable for application map visualizations. | |
| * </p> | |
| * | |
| * @see com.navercorp.pinpoint.web.applicationmap.rawdata.LinkDataDuplexMap | |
| * @see com.navercorp.pinpoint.web.vo.Application | |
| */ | |
| public interface HistogramService { | |
| /** | |
| * Retrieves the application map data based on the specified options. | |
| * | |
| * @param option the options for querying the application map | |
| * @return a {@link LinkDataDuplexMap} containing the application map data | |
| */ | |
| LinkDataDuplexMap selectApplicationMap(MapServiceOption option); | |
| /** | |
| * Extracts the list of source applications from the given application map data. | |
| * | |
| * @param linkDataDuplexMap the application map data | |
| * @return a list of {@link Application} objects representing the source applications | |
| */ | |
| List<Application> getFromApplications(LinkDataDuplexMap linkDataDuplexMap); | |
| /** | |
| * Extracts the list of destination applications from the given application map data. | |
| * | |
| * @param linkDataDuplexMap the application map data | |
| * @return a list of {@link Application} objects representing the destination applications | |
| */ |
intr3p1d
force-pushed
the
add_histogram_api
branch
from
b75ad8a to
2155d69
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12489 +/- ##
============================================
- Coverage 33.59% 33.57% -0.02%
- Complexity 10738 10741 +3
============================================
Files 3941 3943 +2
Lines 91809 91872 +63
Branches 9553 9558 +5
============================================
+ Hits 30844 30847 +3
- Misses 58329 58386 +57
- Partials 2636 2639 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.