Skip to content

Conversation

@shichaoyuan
Copy link
Contributor

  • The issue of the current replacement algorithm repeatedly replacing strings containing {} has been fixed.
  • Unnecessary regular expression replacements have been replaced with simple string replacements.

For replaceParam("from {} to {}", "a={}", "b={}"), the correct output should be from a={} to b={}, not from a=b={} to {}.

Closes apache/skywalking#13591

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 fixes a bug in the AbstractLogger.replaceParam method where replacement strings containing the {} marker would be incorrectly processed, causing repeated replacements. The fix replaces the regex-based approach with a more efficient StringBuilder-based implementation that correctly handles replacement strings containing {} by tracking the position after each replacement.

  • Replaces regex-based string replacement with StringBuilder for efficiency and correctness
  • Adds comprehensive test coverage for both normal cases and the specific bug scenario
  • Updates CHANGES.md to document the fix

Reviewed changes

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

File Description
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/logging/core/AbstractLogger.java Refactored replaceParam method to use StringBuilder instead of regex, fixing the double-replacement bug and improving performance
apm-sniffer/apm-agent-core/src/test/java/org/apache/skywalking/apm/agent/core/logging/core/AbstractLoggerTest.java Added new test file with test cases for normal parameter replacement and the specific bug case with {} in replacement strings
CHANGES.md Added changelog entry documenting the fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wu-sheng wu-sheng merged commit c30b98d into apache:main Nov 26, 2025
208 of 262 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] The replaceParam method of AbstractLogger behaves unexpectedly when the replaced string contains a replacement marker

2 participants