Skip to content

Conversation

itsarijitray
Copy link
Contributor

A summary of your pull request, including the what change you're making and why.

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
  • [Segmenters] Tested in the staging environment
  • [Segmenters] [If applicable for this change] Tested for regression with Hadron.

Copy link

codecov bot commented Jul 28, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.03%. Comparing base (80fd723) to head (987fc7b).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...stination-actions/src/destinations/kafka/errors.ts 25.00% 12 Missing ⚠️
...estination-actions/src/destinations/kafka/utils.ts 71.42% 3 Missing and 1 partial ⚠️
...ation-actions/src/destinations/kafka/send/index.ts 50.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3114      +/-   ##
==========================================
+ Coverage   79.68%   80.03%   +0.34%     
==========================================
  Files        1149     1203      +54     
  Lines       21194    24046    +2852     
  Branches     4083     4873     +790     
==========================================
+ Hits        16889    19245    +2356     
- Misses       3604     4079     +475     
- Partials      701      722      +21     

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

@joe-ayoub-segment
Copy link
Contributor

hi @itsarijitray please ping me over Slack when you'd like me to review.

@itsarijitray itsarijitray requested a review from Copilot August 14, 2025 11:23
Copy link
Contributor

@Copilot 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 adds Kafka response emulation functionality to track HTTP requests/responses for the Kafka destination action. The change introduces error handling, HTTP response emulation, and comprehensive Kafka error code mappings to provide better observability and error reporting.

  • Adds HTTP response emulation capability to the request client
  • Implements comprehensive Kafka error handling with proper HTTP status code mappings
  • Updates Kafka send operations to use request client for tracking and error handling

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/destination-actions/src/destinations/kafka/utils.ts Adds request client parameter to sendData function and implements HTTP response emulation for successful/failed Kafka operations
packages/destination-actions/src/destinations/kafka/send/index.ts Updates perform functions to pass request client to sendData and return the result
packages/destination-actions/src/destinations/kafka/errors.ts Adds comprehensive Kafka error code mapping and error handling functionality
packages/core/src/request-client.ts Adds emulateHttpResponse option to RequestOptions for HTTP response emulation
packages/core/src/tests/create-request-client.test.ts Adds test coverage for the new HTTP response emulation feature
Comments suppressed due to low confidence (2)

packages/destination-actions/src/destinations/kafka/errors.ts:229

  • The settings.brokers parameter is passed twice to handleKafkaError - once as the url parameter and again as the defaultMessage parameter. The second parameter should be a meaningful error message string, not the brokers array.
      kafkaStatus: 'NOT_ENOUGH_REPLICAS_AFTER_APPEND',

packages/destination-actions/src/destinations/kafka/errors.ts:239

  • The settings.brokers parameter is passed twice to handleKafkaError - once as the url parameter and again as the defaultMessage parameter. The second parameter should be a meaningful error message string, not the brokers array.
      kafkaStatus: 'INVALID_REQUIRED_ACKS',

* @param defaultMessage - Default error message if no specific mapping found
* @returns IntegrationError or RetryableError based on error type
*/
export async function handleKafkaError(
Copy link
Preview

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

The large KafkaErrorMap (lines 14-1107) is not being used in the handleKafkaError function. The function only checks if the error is retriable or not, but doesn't utilize the comprehensive error mapping that was defined. Consider either using the error map to provide more specific error handling or removing the unused mapping.

Copilot uses AI. Check for mistakes.

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.

3 participants