chewbranca commented on code in PR #5213:
URL: https://github.com/apache/couchdb/pull/5213#discussion_r1807073259
##########
src/couch_stats/src/couch_stats.erl:
##########
@@ -49,6 +55,11 @@ increment_counter(Name) ->
-spec increment_counter(any(), pos_integer()) -> response().
increment_counter(Name, Value) ->
+ %% Should maybe_track_local happen before or after notify?
+ %% If after, only currently tracked metrics declared in the app's
+ %% stats_description.cfg will be trackable locally. Pros/cons.
+ %io:format("NOTIFY_EXISTING_METRIC: ~p || ~p || ~p~n", [Name, Op, Type]),
+ ok = maybe_track_local_counter(Name, Value),
Review Comment:
An essential part of the work done in this PR is to ensure that this is as
minimally impactful as possible. We add ets config lookup to check if it's
enabled, and then we perform the same type of simple integer counter bump
utilizing `ets:update_counter` with `distributed_counters` enabled and _no_
concurrent writes to the same key, all writes to any given key are _only_
written to from the process tracking its own work, and then finally the tracker
process will delete the entry from ets when the process dies and closes the
context.
@nickva you did have me change my hard coded atom function to one utilizing
maps (although I did mange to use static maps), so I'll make sure to do some
more testing with the final version to compare. I plan on doing some
comparisons between the start of this PR and the final version, to make sure
there weren't any regressions induced by the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
