- 
        Couldn't load subscription status. 
- Fork 23
Performance test improvements #384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 enhances performance testing capabilities by adding detailed metrics collection including eachBatch processing, memory usage tracking, timing measurements, and offset lag monitoring. The changes consolidate common consumer functionality and expand test coverage to include both eachMessage and eachBatch consumption patterns.
Key changes:
- Extracted common consumer logic into a shared module supporting both eachMessage and eachBatch patterns
- Added comprehensive memory tracking with RSS measurements during test execution
- Enhanced metrics collection including offset lag, processing time, and batch statistics
- Migrated performance test script from Bash to Node.js for better cross-platform compatibility
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description | 
|---|---|
| examples/performance/performance-primitives-common.js | New shared module implementing consumer logic for both eachMessage and eachBatch patterns with enhanced metrics | 
| examples/performance/performance-primitives.js | Refactored to use common consumer module with compatibility wrapper | 
| examples/performance/performance-primitives-kafkajs.js | Updated to use common consumer module and added duration logging | 
| examples/performance/performance-consolidated.js | Added memory tracking, expanded test coverage for both consumption patterns, and enhanced logging | 
| ci/tests/run_perf_test.sh | Removed Bash-based performance test script | 
| ci/tests/run_perf_test.js | New Node.js-based performance test script with enhanced metric extraction | 
| .semaphore/semaphore.yml | Updated to use Node.js performance test script | 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
        
          
                ci/tests/run_perf_test.js
              
                Outdated
          
        
      | const outputConfluentProducerConsumer = runCommand('MODE=confluent MESSAGE_COUNT=50000 node performance-consolidated.js --create-topics --consumer --producer'); | ||
|  | ||
| console.log('Running KafkaJS Producer/Consumer test...'); | ||
| const outputKjsProducerConsumer = runCommand('MODE=kafkajs MESSAGE_COUNT=50000 node performance-consolidated.js --create-topics --consumer --producer'); | 
    
      
    
      Copilot
AI
    
    
    
      Oct 8, 2025 
    
  
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message count has been reduced from 500000 to 50000 compared to the original bash script. This significant reduction (90%) may affect the reliability of performance measurements and should be documented or reconsidered.
| const outputConfluentProducerConsumer = runCommand('MODE=confluent MESSAGE_COUNT=50000 node performance-consolidated.js --create-topics --consumer --producer'); | |
| console.log('Running KafkaJS Producer/Consumer test...'); | |
| const outputKjsProducerConsumer = runCommand('MODE=kafkajs MESSAGE_COUNT=50000 node performance-consolidated.js --create-topics --consumer --producer'); | |
| const outputConfluentProducerConsumer = runCommand('MODE=confluent MESSAGE_COUNT=500000 node performance-consolidated.js --create-topics --consumer --producer'); | |
| console.log('Running KafkaJS Producer/Consumer test...'); | |
| const outputKjsProducerConsumer = runCommand('MODE=kafkajs MESSAGE_COUNT=500000 node performance-consolidated.js --create-topics --consumer --producer'); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes the test faster with bigger messages sizes and doesn't seem to affect reliability of results.
99bbb70    to
    7cd3c3f      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
    
      
        1 similar comment
      
    
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the new features to the test (the rss) and eachBatch. The run on semaphore also looks fine to me, as expected in terms of consume throughput and rss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes suggested
| ret = { | ||
| ...ret, | ||
| ssl: parameters.securityProtocol.toLowerCase().includes('ssl'), | ||
| sasl: { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing it here, at a later point, do
if (parameters.securityProtocol.toLowerCase().includes('sasl')) {
 ret['sasl'] = { ... };
}
So we can have just ssl. And above && condition can be changed also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the confluent performance primitives.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
    
      
        4 similar comments
      
    
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
f2d1c71    to
    1e7236e      
    Compare
  
    …essage count and partition number
6a53af2    to
    e999d4d      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
    
      
        1 similar comment
      
    
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
e999d4d    to
    86f9ebf      
    Compare
  
    86f9ebf    to
    ed0fb03      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
9e73424    to
    ed0fb03      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
    
      
        1 similar comment
      
    
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
15928f1    to
    4209743      
    Compare
  
    
      
        
              This comment has been minimized.
        
        
      
    
  This comment has been minimized.
4209743    to
    9f4461f      
    Compare
  
    
measure eachBatch rate, time and lag, avg and max memory usage