Skip to content

Comments

[#12961] Remove validation annotations from high-frequency collector methods to fix performance regression#12965

Merged
emeroad merged 1 commit intomasterfrom
copilot/fix-f029083e-0629-4996-a3b0-35f8f643c66e
Oct 12, 2025
Merged

[#12961] Remove validation annotations from high-frequency collector methods to fix performance regression#12965
emeroad merged 1 commit intomasterfrom
copilot/fix-f029083e-0629-4996-a3b0-35f8f643c66e

Conversation

Copy link

Copilot AI commented Oct 2, 2025

Summary

Successfully removed Java Bean Validation annotations from high-frequency collector methods to address performance regression reported in issue #12961.

Changes

Details

  • Removed @validated annotation from HbaseTraceService class
  • Removed @Valid annotation from insertSpanChunk method in HbaseTraceService
  • Removed @Valid annotation from insertSpan method in HbaseTraceService
  • Removed @Valid annotation from insertSpanChunk method in TraceService interface
  • Removed @Valid annotation from insertSpan method in TraceService interface
  • Removed @validated annotation from LinkServiceImpl class
  • Removed @notblank annotations from updateOutLink method parameters in LinkService interface
  • Removed @notblank annotations from updateAgentState method parameters in LinkService interface
  • Removed @notblank annotations from updateOutLink method parameters in LinkServiceImpl
  • Removed @notblank annotations from updateAgentState method parameters in LinkServiceImpl

Verification

  • All files pass basic syntax checks
  • Method signatures remain unchanged (except for removed annotations)
  • No functional impact expected as validation is centralized in ServerHeaderReader
  • All commits squashed into a single commit as requested by @emeroad

Co-authored-by: emeroad 7564547+emeroad@users.noreply.github.com

Original prompt

This section details on the original issue you should resolve

<issue_title>Performance regression: extra CPU usage in collector after #10081 changes</issue_title>
<issue_description>Hello,

While upgrading from Pinpoint 2.x to 3.x, I observed that the collector consumes noticeably more CPU for the same workload (in my case, around 20% higher).
After analysis, I found that this overhead is related to issue #10081.
In my case, I confirmed that modifying only the following frequently invoked methods restored the resource consumption to the same level as in the previous version:

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

If possible, could you consider removing annotation-based validation for these high-frequency methods?

Thank you. 🙏 </issue_description>

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

@emeroad Java Bean Validation was added for the legacy Thrift protocol and currently appears redundant.

For the gRPC protocol, centralized validation is performed in ServerHeaderReader, so it seems safe to remove it.

https://github.com/pinpoint-apm/pinpoint/blob/master/grpc/src/main/java/com/navercorp/pinpoint/grpc/server/ServerHeaderReaderV1.java#L61

    @Override
    public Header extract(Metadata headers) {
        final String agentId = headerExtractor.getId(headers, Header.AGENT_ID_KEY);
        String agentName = headerExtractor.getName(headers, Header.AGENT_NAME_KEY);
        if (StringUtils.isEmpty(agentName)) {
            agentName = agentId;
        }

        final String applicationName = headerExtractor.getName(headers, Header.APPLICATION_NAME_KEY, applicationNameMaxLength);
        final long startTime = headerExtractor.getTime(headers, Header.AGENT_START_TIME_KEY);
        final int serviceType = headerExtractor.getServiceType(headers);
        final long socketId = headerExtractor.getSocketId(headers);
        final List<Integer> supportCommandCodeList = headerExtractor.getSupportCommandCodeList(headers);
        final boolean grpcBuiltInRetry = headerExtractor.getGrpcBuiltInRetry(headers);
        final Map<String, Object> properties = metadataConverter.apply(headers);
        return new HeaderV1(name, agentId, agentName, applicationName, serviceType, startTime, socketId, supportCommandCodeList, grpcBuiltInRetry, properties);
    }
@emeroad @koo-taejin PR please~~ @koo-taejin I am going to do it :) Thanks you
Fixes #12961

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@CLAassistant
Copy link

CLAassistant commented Oct 2, 2025

CLA assistant check
All committers have signed the CLA.

Copilot AI changed the title [WIP] Performance regression: extra CPU usage in collector after #10081 changes Remove validation annotations from high-frequency collector methods to fix performance regression Oct 2, 2025
Copilot AI requested a review from emeroad October 2, 2025 02:59
@emeroad
Copy link
Member

emeroad commented Oct 2, 2025

@copilot
Add [#12961] as a prefix to the comment message.

@emeroad emeroad added this to the 3.1.0 milestone Oct 2, 2025
Copy link
Author

Copilot AI commented Oct 2, 2025

@copilot
Add [#12961] as a prefix to the comment message.

Done. Added [#12961] prefix to the commit message in commit 19079e4.

@emeroad
Copy link
Member

emeroad commented Oct 2, 2025

@copilot
제일 마지막 commit을 기준으로 3개의 commit을 하나로 병합해줘

@emeroad
Copy link
Member

emeroad commented Oct 2, 2025

@copilot
4개 commit을 1개 commit으로 병합

Copy link
Author

Copilot AI commented Oct 2, 2025

@copilot
제일 마지막 commit을 기준으로 3개의 commit을 하나로 병합해줘

I attempted to squash the commits, but due to tool limitations (cannot force push), the rebase operation preserves the remote history. To properly squash commits that are already pushed, you'll need to either:

  1. Use git rebase -i ac1dd7f locally, squash the commits, then git push --force
  2. Use GitHub's "Squash and merge" option when merging the PR

The automated tools I have access to don't support force pushing, which is required to rewrite pushed commit history.

@emeroad emeroad force-pushed the copilot/fix-f029083e-0629-4996-a3b0-35f8f643c66e branch from 89c19e1 to 6a52301 Compare October 2, 2025 05:32
@emeroad emeroad changed the title Remove validation annotations from high-frequency collector methods to fix performance regression [#12961] Remove validation annotations from high-frequency collector methods to fix performance regression Oct 2, 2025
Copy link
Author

Copilot AI commented Oct 2, 2025

@copilot
4개 commit을 1개 commit으로 병합

The commits have been successfully squashed into a single commit (6a52301). All changes are now in one commit with the message "[#12961] Remove validation annotations from high-frequency collector methods".

@emeroad emeroad force-pushed the copilot/fix-f029083e-0629-4996-a3b0-35f8f643c66e branch from 6a52301 to d1bb8e3 Compare October 2, 2025 05:36
@emeroad
Copy link
Member

emeroad commented Oct 2, 2025

@copilot
3.0.x brach를 기준으로 새로운 backport PR을 생성해줘

Copy link
Author

Copilot AI commented Oct 2, 2025

@copilot
3.0.x brach를 기준으로 새로운 backport PR을 생성해줘

I don't have the capability to create new pull requests through the available tools. To create a backport PR for 3.0.x, you can:

  1. Manually cherry-pick the commit: git checkout 3.0.x && git cherry-pick d1bb8e3 && git push origin HEAD:backport-12961-to-3.0.x
  2. Then create a PR from the GitHub UI targeting the 3.0.x branch

Alternatively, I can create a local branch with the changes applied to 3.0.x, but I cannot create the PR itself.

Copy link
Member

@emeroad emeroad left a comment

Choose a reason for hiding this comment

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

LGTM

@emeroad emeroad requested a review from Copilot October 2, 2025 05:47
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 removes Java Bean Validation annotations from high-frequency collector methods to address a performance regression that caused ~20% higher CPU usage after issue #10081 changes.

  • Removed @validated class annotations and @Valid/@notblank parameter annotations from collector service methods
  • Added documentation comments referencing the performance issue
  • Maintained all method signatures except for the removed validation annotations

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
TraceService.java Removed @Valid annotations from insertSpanChunk and insertSpan interface methods
HbaseTraceService.java Removed @validated class annotation and @Valid parameter annotations from high-frequency methods
LinkService.java Removed @notblank annotations from updateOutLink and updateAgentState interface method parameters
LinkServiceImpl.java Removed @validated class annotation and @notblank parameter annotations from implementation methods

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

@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 33.28%. Comparing base (ac1dd7f) to head (d1bb8e3).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master   #12965   +/-   ##
=========================================
  Coverage     33.28%   33.28%           
  Complexity    10806    10806           
=========================================
  Files          3992     3992           
  Lines         92936    92936           
  Branches       9696     9696           
=========================================
+ Hits          30936    30937    +1     
+ Misses        59349    59348    -1     
  Partials       2651     2651           

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

@koo-taejin
Copy link
Member

koo-taejin commented Oct 2, 2025

Due to the company’s security policy, I’m unable to click approving this. 😢
Looks good.

@emeroad emeroad merged commit 4debeec into master Oct 12, 2025
6 checks passed
@emeroad emeroad deleted the copilot/fix-f029083e-0629-4996-a3b0-35f8f643c66e 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.

Performance regression: extra CPU usage in collector after #10081 changes

4 participants