Counter API

[repeatedly]

Update version of #1241
We continue to update a patch here.

[repeatedly]

@okkez If you want to test counter-api, use this branch instead.

[okkez]

I've tried to test counter-api but it seems that counter-api is wok in progress.
Fluentd does not call any counter-api (client). So I cannot test counter-api with Fluentd and fluent-plugin-xxx, for now.

I will try to add some ad-hoc patch and test counter-api with Fluentd.

[okkez]

I've noticed some points.

[okkez]

How about splitting host and port for consistency with other plugins such as in_forward and in_monitor_agent?
It is useful to set the default value for those parameters.

config_section :counter_server, multi: false do
  config_param :bind, :string
  config_param :port, :integer
end

[okkez]

How about following? It is useful to set the default value for those parameters.

config_sectio :counter_client, multi: false do
  config_param :host, :string
  config_param :port, :integer
end

[okkez]

How about following?
Fluent::Counter::Client is used by plugin developers and Counter API users. Is this right?
opt ={} does not describe details of parameters. Thus users have to read the code to understand this API.
We may also need plugin helper to create Counter API client for plugins.

def initialize(event_loop: Coolio::Loop.new, log: $log, host: '127.0.0.1', port: 4321)
  @event_loop = event_loop
  @host = host
  @port = port
  @log = log
  @conn = Connection.connect(@host, @port, method(:on_message))
  # ...
end

[okkez]

@counter_endpoint is removed in 056a344.

[okkez]

How about like following?

      def inc(params, options: {})
        exist_scope!
        params = [params] unless params.is_a?(Array)
        res = send_request('inc', @scope, params, options)
        if options[:async]
          if block_given?
            Thread.start do
              yield res.get
            end
          else
            res
          end
        else
          if block_given?
            yield res.get
          else
            res.get
          end
        end
      end

We can use this as following:

@counter_client = ...
@counter_client.inc({ name: "counter", value: 1}, options: { async: true }) do |response|
   # use response here
end

In the original code, we must handle future instance always when we send request asynchronously.

[repeatedly]

Added suggested API.

[okkez]

How about changing the return value to Response object from plain Hash?

class Response
   attr_reader :errors, :data
   def initialize(result)
      @errors = result["errors"]
      @data = result["data"]
      # ...
   end

  def success?
    @errors.nil? || @errors.empty?
  end

  def error?
    !success?
  end
end

It is useful to check Counter API response as following:

@counter_client.inc({ name: "counter", value: 1}, options: { async: true }) do |response|
   if response.success?
     log.debug("Update counter successfully")
   else
     log.warn("Counter API error: #{response.errors}")
   end
end

Original code:

future = @counter_client.inc({ name: "counter", value: 1}, options: { async: true })
Thread.start do
  response = future.get
  if future.errors?
     log.error("failure")
  else
     log.debug("success")
  end
end

[repeatedly]

Looks good. I will apply the changes soon.

[repeatedly]

@okkez Do you have other suggestion? If not, I have a plan to merge this PR.

[okkez]

@repeatedly I want a shorthand for @counter_client.inc({ name: "name", value: 1 }, options: { async: true }). It is annoying to specify options: { async: true } for each asynchronous request.

How about that the counter client sends an asynchronous request by default?

Or, how about that plugin helper creates an asynchronous client?

async_client = counter_client_create(scope: "datacounter", async: true)
async_client.inc(name: "name", value: 1) # send an asynchronous request
async_client.inc(name: "name", value: 1, options: { async: false }) # send a synchronous request

Or else...?
Anyway, I want a shorthand to send asynchronous requests.

[repeatedly]

How about that the counter client sends an asynchronous request by default?

I see. One idea is removing sync API. Adding .get is low cost and it is not messy in user code.

future = client.inc(name: "name", value: 1)  # async
result = client.inc(name: "name", value: 1).get  # get result in sync

How about this?

[okkez]

Looks good to me 👍

[repeatedly]

Remove sync API

[repeatedly]

Merged. We will improve Counter API with actual plugins.