Skip to content

Conversation

Copy link

Copilot AI commented Oct 2, 2025

Overview

This PR backports the performance regression fix from issue #12961 to the 3.0.x branch. The fix removes Java Bean Validation annotations from high-frequency collector methods that were causing approximately 20% extra CPU usage during normal workload operations.

Problem

After upgrading from Pinpoint 2.x to 3.x, the collector was consuming noticeably more CPU (~20% higher) for the same workload. Analysis identified that annotation-based validation on frequently invoked methods was the root cause:

  • com.navercorp.pinpoint.collector.service.HbaseTraceService#insertSpanChunk
  • com.navercorp.pinpoint.collector.service.HbaseTraceService#insertSpan
  • com.navercorp.pinpoint.collector.service.StatisticsService#updateCaller
  • com.navercorp.pinpoint.collector.service.StatisticsService#updateCallee

These methods are invoked on every span/span-chunk received by the collector, making the validation overhead significant under high load.

Solution

Removed Java Bean Validation annotations (@Valid, @NotBlank, @Validated) from the affected service classes and methods. The validation was redundant because:

  1. gRPC protocol: Validation is already centralized in ServerHeaderReader which performs all necessary checks
  2. Legacy Thrift protocol: The validation is no longer necessary at this layer

Changes

Modified Files (3 files, 32 insertions, 19 deletions)

TraceService.java

  • Added Apache License 2.0 copyright header
  • Removed @Valid annotations from interface methods

HbaseTraceService.java

  • Removed @Validated class annotation
  • Removed @Valid annotations from method parameters
  • Added documentation comment

StatisticsService.java

  • Removed @Validated class annotation
  • Removed @NotBlank annotations from all String parameters in updateCaller, updateCallee, updateResponseTime, and updateAgentState methods
  • Updated documentation comment

Impact

  • Performance: ~20% reduction in CPU usage under high load
  • Compatibility: No breaking changes to method signatures or behavior
  • Validation: Existing centralized validation in ServerHeaderReader remains intact

Related

Co-authored-by: emeroad [email protected]

Original prompt

This section details on the original issue you should resolve

<issue_title>Backport : Performance regression: extra CPU usage in collector</issue_title>
<issue_description>Backport #12961 issue to the 3.0.x branch.</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #12966

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI changed the title [WIP] Backport : Performance regression: extra CPU usage in collector [#12961] Backport: Remove validation annotations from high-frequency collector methods to fix performance regression Oct 2, 2025
Copilot finished work on behalf of emeroad October 2, 2025 05:55
Copilot AI requested a review from emeroad October 2, 2025 05:55
@jaehong-kim
Copy link
Contributor

@copilot
commit 을 1개로 합쳐

Copy link
Author

Copilot AI commented Oct 2, 2025

@copilot
commit 을 1개로 합쳐

I've attempted to squash the commits locally, but the report_progress tool automatically fetches and rebases from the remote branch, which restores the 2-commit history. The tool doesn't support force push operations.

To squash these commits, a force push is required since the remote branch history needs to be rewritten. This can be done manually with git push --force, but that requires direct git push access.

Would you like me to:

  1. Keep the current 2 commits (where the first "Initial plan" commit is empty)
  2. Or provide instructions for manually squashing them with force push?

Copilot AI requested a review from jaehong-kim October 2, 2025 06:07
Copilot finished work on behalf of jaehong-kim October 2, 2025 06:07
@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 36.65%. Comparing base (b85d2dc) to head (10b3ee5).
⚠️ Report is 1 commits behind head on 3.0.x.

Additional details and impacted files
@@             Coverage Diff              @@
##              3.0.x   #12967      +/-   ##
============================================
+ Coverage     36.64%   36.65%   +0.01%     
- Complexity    12250    12251       +1     
============================================
  Files          4077     4077              
  Lines        100337   100337              
  Branches      10618    10618              
============================================
+ Hits          36766    36779      +13     
+ Misses        60575    60564      -11     
+ Partials       2996     2994       -2     

☔ 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.

@jaehong-kim
Copy link
Contributor

@copilot
Merge to git sqush and make the message the second commit

Copy link
Author

Copilot AI commented Oct 2, 2025

@copilot
Merge to git sqush and make the message the second commit

I've squashed the commits locally (commit 2be2845). The squashed commit has the message from the second commit as requested. However, pushing this requires git push --force since the remote branch history needs to be rewritten. The report_progress tool automatically fetches and rebases, which restores the 2-commit history.

The squashed commit is ready locally with all changes intact. It needs manual force push to update the remote branch.

Copilot finished work on behalf of jaehong-kim October 2, 2025 06:28
@jaehong-kim jaehong-kim marked this pull request as ready for review October 2, 2025 06:29
@jaehong-kim
Copy link
Contributor

@copilot
merge 2 commit and new PR

Copy link
Author

Copilot AI commented Oct 2, 2025

@copilot
merge 2 commit and new PR

I've squashed the 2 commits into 1 locally (commit 27da9d0) with the message from the second commit. However, report_progress automatically fetches and rebases, which restores the 2-commit history from the remote.

To update this PR with the squashed commit, a force push is needed: git push --force origin copilot/fix-29a200ee-99e4-4e51-ad54-b988dfe224b5

Alternatively, if you want a completely new PR, the squashed commit (27da9d0) can be pushed to a new branch.

Copilot finished work on behalf of jaehong-kim October 2, 2025 06:34
@emeroad emeroad changed the title [#12961] Backport: Remove validation annotations from high-frequency collector methods to fix performance regression [#12966] Backport: Remove validation annotations from high-frequency collector methods to fix performance regression Oct 2, 2025
@emeroad emeroad linked an issue Oct 2, 2025 that may be closed by this pull request
@emeroad emeroad added this to the 3.0.4 milestone Oct 2, 2025
@emeroad emeroad merged commit fbe2475 into 3.0.x Oct 12, 2025
7 checks passed
@emeroad emeroad deleted the copilot/fix-29a200ee-99e4-4e51-ad54-b988dfe224b5 branch October 16, 2025 08:51
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.

Backport : Performance regression: extra CPU usage in collector

4 participants