Skip to content
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

feat: add an error callback option to provide the affected data #22

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

chunhuiteng
Copy link

Description

Currently, when there is an error sending data to Rudderstack, although the caller's callback method would be called, there is no mechanism for the caller to discover which data is affected, thus no way to take the precise action(s) for it.

This change add another callback option which would provide the caller with the data affected by the error, so the caller could act accordingly, such as log the data, or retransmit the data later.

The change is backward compatible. The existing clients still work as they are configured, but could be changed to use this new callback option.

@contributor-support
Copy link

Thank you @chunhuiteng for contributing this PR.
Please sign the Contributor License Agreement (CLA) before merging.

@etsenake
Copy link

@chunhuiteng This looks great! What are your thoughts on the mechanism for retrying? if we wanted to send all the messages again, since it would include identify, tracks and lots of other calls it might be a bit of a hassle to have the user of said messages have to figure out the right method to call with those messages for retrying in addition to the re-processing that would also happen. I think it might be worth adding a send_batch_messages method to the client such that any user could pass it all the messages in one shot without having to to worry about further processing or re-processing that might happen

@etsenake
Copy link

etsenake commented Nov 22, 2024

@chunhuiteng This looks great! What are your thoughts on the mechanism for retrying? if we wanted to send all the messages again, since it would include identify, tracks and lots of other calls it might be a bit of a hassle to have the user of said messages have to figure out the right method to call with those messages for retrying in addition to the re-processing that would also happen. I think it might be worth adding a send_batch_messages method to the client such that any user could pass it all the messages in one shot without having to to worry about further processing or re-processing that might happen

I'm thinking something more like this:

require 'rudder/analytics/utils'
require 'rudder/analytics/logging'
require 'rudder/analytics/message_batch'
require 'rudder/analytics/transport'

module Rudder
  class Analytics
    class BatchWorker
      include Rudder::Analytics::Utils
      include Rudder::Analytics::Logging

      def initialize(config)
        @write_key = config.write_key
        @on_error = config.on_error
        @transport = Transport.new(config)
      end

      def process_batch(messages)
        batch = MessageBatch.new(messages.length)

        messages.each do |message|
          message[:messageId] ||= uid
          symbolize_keys!(message)
          batch << message
        end

        res = @transport.send(@write_key, batch)
        unless res.status == 200
          @on_error.call(res.status, res.error) 
          @on_error_with_messages.call(res.status, res.error, @batch.messages)
        end
        res
        ensure
          @transport.shutdown
        end
    end
  end
end


### then we add a method to the client.rb which would make it available to the Rudder::Analytics instances
def send_batch(messages)
  batch_worker = BatchWorker.new(@config)
   batch_worker.process_batch(messages)
end

@chunhuiteng
Copy link
Author

@chunhuiteng This looks great! What are your thoughts on the mechanism for retrying? if we wanted to send all the messages again, since it would include identify, tracks and lots of other calls it might be a bit of a hassle to have the user of said messages have to figure out the right method to call with those messages for retrying in addition to the re-processing that would also happen. I think it might be worth adding a send_batch_messages method to the client such that any user could pass it all the messages in one shot without having to to worry about further processing or re-processing that might happen

@etsenake Great idea. I will try to add such a method in another PR.

@saikumarrs
Copy link
Member

Hello @chunhuiteng 👋
Thanks for raising the PR.

Please fix the PR title as per the conventional commits format.

@chunhuiteng chunhuiteng changed the title Add an option (on_error_with_messages) to provide a callback option to get the data affected by the error feat: add an option (on_error_with_messages) to provide a callback option to get the data affected by the error Dec 3, 2024
@chunhuiteng chunhuiteng changed the title feat: add an option (on_error_with_messages) to provide a callback option to get the data affected by the error feat: add an error callback option to provide the affected data Dec 3, 2024
@saikumarrs saikumarrs merged commit af48c1a into rudderlabs:master Dec 6, 2024
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants