Dear Erik,

thanks for the detailed explanation and the heads-up. A few comments
and questions below.

On 26/08/15 17:49, "Erik Jasiak" <mjasiak(a)pivotal.io
<mailto:mjasiak(a)pivotal.io>> wrote:

We’ve decided to fix the collector bug to not overwrite these
tags.However, we wanted to explain the impact:If this change will
cripple your existing CF monitoring (i.e. the possible change in
metrics data will hurt you, and you’d rather call the bug the
‘intended behavior’) please let us know ASAP.


Will the change you are doing now in any way affect the already
existing metrics and what users of the graphite plugin see as their
names? Names are computed from the tags ‘deployment’, ‘job’, ‘index’,
‘ip’, ‘key’ [1].

We currently use the collector to forward metrics in graphite format
and built alerting and dashboards around this. So the alerts and
dashboards contain metric names in their queries, such as
“CF-landscape.Router.*.<metric name>”. If the names change, it *will*
break the queries we have and render the alerts and dashboards useless
– which might be somewhat transparent for dashboards, because you no
longer see data, but is a disaster for alerts when you assume
everything is fine if no event occurs.

I am aware that at some point in time we have to migrate to a
graphite-firehose-nozzle and I am fine if we have to adapt metric
names in alerts and dashboards. However, as long as I don’t do that
migration, I would expect that our setup continues to work as it does
today.


I’m also not sure about the detailed analysis:

the collector will override the tag provided by the varz endpoint
("job:api_z1") with the tag computed as a result of the
registration message on NATS ("job:CloudController"). As a result,
we may end up with unexpected tags in our downstream consumers of
metrics.


This is only about metrics which get added now, right? Because for all
metrics existing today, I would not call the current behavior
“unexpected” - this is what I’m using right now.

*Potential Ramifications:*

Although this change is pretty small, we could think of a
hypothetical situation where this update changes the tags emitted
by the collector. If a metric includes one of these tags already,
it will no longer be overridden. As a result, the downstream
consumer would see a different tag.


This part also seems to be targeted to authors providing new metrics.
Because, again, for the already existing metrics I as a consumer don’t
know about the tags with which the metrics are emitted from the
components currently, all I know about is what I’m getting from the
collector.

*TL;DR*: As long as for none of the existing metrics the tags
‘deployment’, ‘job’, ‘index’, ‘ip’, ‘key’ change, this change should
be fine from our point of view. If they change, this change is a
breaking one and I’d rather see this bug not fixed in the collector,
but adapt my existing alerts and dashboards once I switch to the firehose.

Warm regards
Marco

[1]
https://github.com/cloudfoundry/collector/blob/master/lib/collector/historian/graphite.rb#L25-L42