Add service tag to health metrics #2154
Conversation
marcotc
force-pushed
the
health-metrics-servie
branch
from
0f86f75 to
1a8ecc8
Compare
| # Return dupes, so that the constant isn't modified, | ||
| # and defaults are unfrozen for mutation in Statsd. | ||
| super.tap do |options| | ||
| options[:tags] = options[:tags].dup |
There was a problem hiding this comment.
I noticed we also do the dup in Core::Runtime::Metrics#default_metric_options but is this really needed?
As far as I see it, super here is referring to Datadog::Core::Metrics::Options#default_metric_options which already dups the frozen tags so I don't think we need to on any of the child classes...
| options[:tags] << @tags | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # Cache tag strings to avoid recreating them on every flush operation. | ||
| def compile_tags! | ||
| return unless @service | ||
|
|
||
| "#{Environment::Ext::TAG_SERVICE}:#{@service}".freeze | ||
| end |
There was a problem hiding this comment.
Hmm is it me or is this code looking deceptively generic, but not actually? E.g. from a high level, there's @tags = compile_tags!, which looks generic.
But then compile_tags! is not a list but a single tag; and compile_tags! doesn't even run if no @service is around.
I would suggest picking either making this specific (use a @service_tag) or really generic (actually have a list in compile_tags!).
(I actually somewhat favor just doing @service_tag = ... in the constructor and then referring it directly in #default_metric_options).
There was a problem hiding this comment.
I think the chain of condition introduce unnecessary cognitive burden and this method does not deserved to be extracted as private method, since it is just a simple conditional being invoke only once during initialisation.
| def build_health_metrics(settings) | ||
| settings = settings.diagnostics.health_metrics | ||
| options = { enabled: settings.enabled } | ||
| options[:statsd] = settings.statsd unless settings.statsd.nil? | ||
| health_settings = settings.diagnostics.health_metrics | ||
|
|
||
| Core::Diagnostics::Health::Metrics.new(**options) | ||
| Core::Diagnostics::Health::Metrics.new( | ||
| service: settings.service, | ||
| enabled: health_settings.enabled, | ||
| statsd: health_settings.statsd | ||
| ) | ||
| end |
There was a problem hiding this comment.
This PR baffles me in a way. That is, this feature is SO obvious, that I cannot think why we didn't have it from the get-go. Am I missing something? How would this be usable without a service tag, unless customers had exactly 1 service reporting to their account?
| let(:service) { nil } | ||
|
|
||
| it 'does not set the statsd service tag' do | ||
| expect(statsd).to_not receive(:count).with(any_args, tags: array_including(include('service:'))) |
There was a problem hiding this comment.
Usually, I would avoid using to_not with detail constraints since this leads to weaker assertion.
I would prefer simply a more deterministic assertion to prove the non-existence of the missing element.
expect(statsd).to receive(:count).with('api_requests', 1, tags: ['...', '...'])
RSpec does provided hash_excluding but not array_excluding. There is work around with negation matcher is working but not satisfying in terms of semantic.
RSpec::Matchers.define_negated_matcher :exclude, :include
expect(statsd).to receive(:count).with(any_args, tags: array_including(exclude('service:')))
Yeah... asserting something's non-existence is always kinda awkward, since the appearance could be in various form(such as 'service:nil').
| @@ -3,9 +3,12 @@ | |||
| require 'spec_helper' | |||
| require 'ddtrace' | |||
| require 'datadog/core/diagnostics/health' | |||
| require 'datadog/statsd' | |||
|
|
|||
| RSpec.describe Datadog::Core::Diagnostics::Health::Metrics do | |||
There was a problem hiding this comment.
Should this file being rename to spec/datadog/core/diagnostics/health/metrics_spec.rb?
| @@ -30,6 +37,26 @@ class Metrics < Core::Metrics::Client | |||
| gauge :queue_max_length, Ext::Health::Metrics::METRIC_QUEUE_MAX_LENGTH | |||
| gauge :queue_spans, Ext::Health::Metrics::METRIC_QUEUE_SPANS | |||
| gauge :sampling_service_cache_length, Ext::Health::Metrics::METRIC_SAMPLING_SERVICE_CACHE_LENGTH | |||
|
|
|||
| def default_metric_options | |||
There was a problem hiding this comment.
Is this public method tested somewhere?
I supposed it would be a better candidate for verifying service tag, compared a more integrated #api_requests test.
Currently health metrics are sent without a
servicetag, with only theenvtag.This means that all services in the same
envwill get their health metrics merged together. This makes these metrics not useful.This PR adds the
servicetag, collected fromc.service, to all statsd metrics generated for health metrics.No service name being set (
c.service == nil) is still supported (metrics will be send successfully without aservice), but not very useful in practice.For Runtime Metrics, this currently works because runtime metrics collect service names from spans being flushed, thus populating an internal list of services.