shackle icon indicating copy to clipboard operation
shackle copied to clipboard

More telemetry events

Open rkallos opened this issue 2 years ago • 2 comments

A couple of changes for your consideration:

  1. Storing Request values (understood by implementors of shackle_client) in shackle_queue, and passing them to shackle_telemetry:reply/4 and shackle_telemetry:timeout/2. I added this to get the HTTP status code and URL path from buoy.
  2. Adding shackle_telemetry:connection_error/2 and shackle_telemetry:queued_time/2.
  3. Using erlang:monotonic_time/{0,1} to measure elapsed time, instead of os:timestamp/0 and timer:now_diff/2. This is specifically called out in the Time and Time Correction in Erlang, though it mentions erlang:timestamp/0. It does, however, recommend using erlang:monotonic_time/0 and subtraction to measure elapsed time between events.

I got rid of a couple of ?WARN invocations because they can be pretty spammy. I'm not sure what the best approach is to handling these log messages, or whether they can be adequately replaced with metrics. I'd be happy to add them back if you want.

rkallos avatar Aug 02 '23 21:08 rkallos

I think it might be a good idea to also include a shackle_telemetry module and add a section int he ready that explain how to enable it (e.g. setup the hook and add the telemetry dependency).

lpgauth avatar Aug 15 '23 13:08 lpgauth

@rkallos It's much better now. Still, I'm wondering if we could go further, to use macros like these:

process_responses([], _State) ->
    ok;     
process_responses([{ExtRequestId, Reply} | T], #state {
        client = Client,
        id = Id,
        queue = Queue
    } = State) ->

    ?EVT_REPLY(Client),
    case shackle_queue:remove(Queue, Id, ExtRequestId) of
        {ok, #cast {timestamp = Timestamp} = Cast, TimerRef} ->
            ?EVT_RESP_FOUND(Client, Timestamp),
            erlang:cancel_timer(TimerRef),
            reply(Reply, Cast, State);
        {error, not_found} ->
            ?EVT_RESP_NOT_FOUND(Client),
            ok
    end,
    process_responses(T, State).

and define them either for telemetry calls:

-define(EVT_REPLY(Client),
    telemetry:execute(
        [shackle, replies],
        #{count => 1},
        #{client => Client}
    )
).
    
-define(EVT_RESP_FOUND(Client, Timestamp), begin
    telemetry:execute(
        [shackle, found],
        #{count => 1},
        #{client => Client}
    ),  
    telemetry:execute(
        [shackle, reply],
        #{duration => timer:now_diff(os:timestamp(), Timestamp)},
        #{client => Client}
    )   
end).       
            
-define(EVT_RESP_NOT_FOUND(Client),
    telemetry:execute(
        [shackle, not_found],
        #{count => 1},
        #{client => Client}
    )
).

or for legacy shackle metrics:

-define(EVT_REPLY(Client),
    (fun
        ({Mod, Fun}) ->
            Mod:Fun(Client, counter, <<"replies">>);
        (_) ->
            ok
    end) (shackle_hooks:handler())
).  

-define(EVT_RESP_FOUND(Client, Timestamp),
    (fun
        ({Mod, Fun}) ->
            Mod:Fun(Client, counter, <<"found">>),
            Diff = timer:now_diff(os:timestamp(), Timestamp),
            Mod:Fun(Client, timing, <<"reply">>, Diff);
        (_) ->
            ok
    end) (shackle_hooks:handler())
).  

-define(EVT_RESP_NOT_FOUND(Client),
    (fun
        ({Mod, Fun}) ->
            Mod:Fun(Client, counter, <<"not_found">>, 1);
        (_) ->
            ok
    end) (shackle_hooks:handler())
).

The project can use some technique to define at compile time which hooks.hrl to use, allowing client's entirely custom solutions (for example, precompiled functions saved to persistent_term).

x0id avatar Aug 17 '23 03:08 x0id