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.

Sign up for GitHub

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

Jump to bottom

Add stats formatting for riak-admin and http stats output #304

Closed
wants to merge 22 commits into from
Closed

Add stats formatting for riak-admin and http stats output #304

wants to merge 22 commits into from

Conversation

bowrocker
Copy link
Contributor

YZ stats are not currently displayed on the riak http stats endpoint, or by riak-admin status. Add YZ stats formatting appropriate for console output with 'riak-admin status' and the htts /stats endpoint. The stats namespace for Search is transformed to /search/[query | index]/... instead of 'yokozuna'.

This PR also depends on changes to riak_kv in order to publish the stats to the 2 interfaces:
basho/riak_kv#839

Example stats output:

search_stat_ts : 1392310824
search_query_fail_count : 0
search_query_fail_one : 0
search_index_throughput_count : 0
search_index_throughput_one : 0
search_index_fail_count : 0
search_index_fail_one : 0
search_query_throughput_count : 0
search_query_throughput_one : 0
search_query_latency_min : 0.0
search_query_latency_max : 0.0
search_query_latency_median : 0.0
search_query_latency_percentile_95 : 0.0
search_query_latency_percentile_99 : 0.0
search_index_latency_min : 0.0
search_index_latency_max : 0.0
search_index_latency_median : 0.0
search_index_latency_percentile_95 : 0.0
search_index_latency_percentile_99 : 0.0

@bowrocker bowrocker mentioned this pull request Feb 13, 2014
Closed
@rzezeski
Copy link
Contributor

  • code review
  • compile/eunit
  • dialyzer
  • riak test
  • manual check of HTTP stats
  • manual check of riak-admin status

rzezeski
rzezeski reviewed Feb 18, 2014
View reviewed changes
src/yz_stat.erl
%% @doc Return formatted stats
-spec get_formatted_stats() -> [term()].
get_formatted_stats() ->
case yokozuna:is_enabled(index) andalso ?YZ_ENABLED of
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the yokozuna:is_enabled(index) check. That check is to allow users to just disabled indexing or querying without disabling all of Yokozuna. We will want to be able to retrieve stats even if index is disabled.

@rzezeski
Copy link
Contributor

I think we should rename the internal stat names to query as well to be consistent with external.

Also, looking at Riak KV it seems it stores the formatted stats in the cache. In fact, KV considers the formatted stats to be "legacy". I think this was because at one point @russelldb was working on moving to the new system but we still needed to present the stats in the old way. Thus riak_kv_stat_bc was created to map the legacy stat names to the new stat namespace. riak_kv_stat_bc:produce_stats is what is registered with the stat cache. Perhaps we should also cache the formatted stats to be consistent. It looks like we may also be able to utilize the riak_kv_stat_bc:bc_stat code since it exports all functions. This would remove the need for all the manual pattern matching and replace it with a list like that found in riak_kv_stat_bc:legacy_stats_map (I wouldn't call it legacy though in this case, maybe just stats_map). This would avoid repeating code especially as we add more stats.

@rzezeski rzezeski mentioned this pull request Feb 18, 2014
Merged
rzezeski
rzezeski reviewed Feb 20, 2014
View reviewed changes
src/yz_stat.erl
@@ -30,7 +30,7 @@
%% -type microseconds() :: integer().
-define(SERVER, ?MODULE).
-define(NOTIFY(A, B, Type, Arg),
folsom_metrics:notify_existing_metric({?YZ_APP_NAME, A, B}, Arg, Type)).
folsom_metrics:notify_existing_metric({search, A, B}, Arg, Type)).
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure all other systems using stats use the application name as first part of namespace. Part of me likes having the root of the namespace be search as that's what yokozuna is for but a bigger part of me wants to stay consistent with other areas of Riak which all namespace based on application.

Change it back to ?YZ_APP_NAME. The renaming will happen in the stats_map.

@rzezeski
Copy link
Contributor

The yz_stats test is failing. Let me know if you have trouble
running the riak tests. This particular PR is a bit tricker than just
running verify.sh because you need a custom riak_kv. Also, you'll
want to merge develop into this branch as it's too far behind to work.

The riak-admin status works:

# ~/yz-verify/rt/riak_yz/dev/dev2/bin/riak-admin status | grep search
riak_search_version : <<"1.4.1-22-ga3893df">>
search_query_throughput_count : 0
search_query_throughput_one : 0
search_query_latency_min : 0
search_query_latency_max : 0
search_query_latency_median : 0
search_query_latency_95 : 0
search_query_latency_99 : 0
search_query_latency_999 : 0
search_index_throughput_count : 0
search_index_throughtput_one : 0
search_index_fail_count : 0
search_index_fail_one : 0
search_index_latency_min : 0
search_index_latency_max : 0
search_index_latency_median : 0
search_index_latency_95 : 0
search_index_latency_99 : 0
search_index_latency_999 : 0

But there was an error for HTTP stats.

# curl 'http://localhost:10018/stats'
<html><head><title>500 Internal Server Error</title></head><body><h1>Internal Server Error</h1>The server encountered an error while processing this request:<br><pre>{error,
    {error,function_clause,
        [{mochijson2,json_encode_string,
             [{yokozuna,query,throughput},{encoder,null,false}],
             [{file,"src/mochijson2.erl"},{line,173}]},
         {mochijson2,'-json_encode_proplist/2-fun-0-',3,
             [{file,"src/mochijson2.erl"},{line,166}]},
         {lists,foldl,3,[{file,"lists.erl"},{line,1248}]},
         {mochijson2,json_encode_proplist,2,
             [{file,"src/mochijson2.erl"},{line,170}]},
         {riak_kv_wm_stats,produce_body,2,
             [{file,"src/riak_kv_wm_stats.erl"},{line,76}]},
         {webmachine_resource,resource_call,3,
             [{file,"src/webmachine_resource.erl"},{line,186}]},
         {webmachine_resource,do,3,
             [{file,"src/webmachine_resource.erl"},{line,142}]},
         {webmachine_decision_core,resource_call,1,
             [{file,"src/webmachine_decision_core.erl"},{line,48}]}]}}</pre><P><HR><ADDRESS>mochiweb+webmachine web server</ADDRESS></body></html>

@rzezeski
Copy link
Contributor

Something weird is going on here. The commits all occur twice with different hashes. Perhaps a bad rebase?

@bowrocker
Copy link
Contributor Author

New PR:

#318

@bowrocker bowrocker closed this Feb 22, 2014
@rzezeski rzezeski deleted the bugfix/jra/yz_stats branch June 25, 2014 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bowrocker @rzezeski