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

More telemetry events #133

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

rkallos
Copy link
Contributor

@rkallos rkallos commented Aug 2, 2023

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.

@lpgauth
Copy link
Owner

lpgauth commented Aug 15, 2023

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

Comment on lines 118 to 120
shackle_telemetry:send(Client, iolist_size(Data)),
shackle_events:send(Client, iolist_size(Data)),
Copy link

Choose a reason for hiding this comment

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

In either case, it is not a good idea to calculate the Data size here, until we know we will use it. It is better to use macro in general and pass Data to telemetry/events function to let it calc size only if needed (if dynamic handlers exist).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point!

I introduced a macro in shackle_events that will avoid computing these values and terms if there is no configured shackle_hooks handler.

@x0id
Copy link

x0id commented Aug 17, 2023

@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).

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.

3 participants