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

converts timers to milliseconds as statsd timers explicitly say to re… #39

Merged
merged 1 commit into from
Nov 12, 2018

Conversation

ETetzlaff
Copy link
Contributor

Statsd docs specify timers should be reported in milliseconds. This converts the time reportings to milliseconds for finer granularity.

@minond
Copy link

minond commented Nov 9, 2018

The harness-protobuf-nats gem will take a given delay/duration value and pass it as is for server metrics (https://github.com/mxenabled/harness-protobuf-nats/blob/1347976770d809ce190e5a44cea97015f1f58b4b/lib/harness/protobuf/nats.rb#L17). The problem is that this delay is in seconds,

enqueued_at = ::Time.now
was_enqueued = thread_pool.push do
begin
# Instrument the thread pool time-to-execute duration.
processed_at = ::Time.now
::ActiveSupport::Notifications.instrument("server.thread_pool_execution_delay.protobuf-nats",
(processed_at - enqueued_at))

But it should be in milliseconds according to the gem we're using, https://github.com/reinh/statsd/blob/master/lib/statsd.rb#L392-L402

Right now this is an issue with the protobuf-nats.server.thread_pool_execution_delay metric and the protobuf-nats.server.request_duration metric. The client.request_duration.protobuf-nats metric is able to get around this since it will use an ::ActiveSupport::Notifications::Event and get the duration using Event#duration, which works on milliseconds (https://api.rubyonrails.org/classes/ActiveSupport/Notifications/Event.html#method-i-duration). Other gems, like harness-action_subscriber, do something similar.

@film42 film42 merged commit d302b71 into abrandoned:master Nov 12, 2018
@film42
Copy link
Collaborator

film42 commented Nov 12, 2018

Released as v0.10.1.

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