Count instances managed by each instance template

Count instances managed by each instance template

BUG=664296
Committed: https://github.com/luci/luci-py/commit/d5d2108ec34c507defdc28af67954f9eafe2e5d4

[smut, 2016-11-21 22:26:59]

smut@google.com changed reviewers:
+ sergeyberezin@chromium.org

[smut, 2016-11-21 22:26:59]

With new types of instances coming online (see bug), we should replace the
aggregated count of all instances with separate counts for each type of
instance.

[Sergey Berezin (google), 2016-11-23 02:28:25]

sergeyberezin@google.com changed reviewers:
+ sergeyberezin@google.com

[Sergey Berezin (google), 2016-11-23 02:28:26]

LGTM + comment.

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
File appengine/gce-backend/metrics.py (right):

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
appengine/gce-backend/metrics.py:33:
'machine_provider/gce_backend/instances/v2',
I'd name it in a more self-descriptive way, e.g. .../instances_by_template.
Or, even better, just add a new field to the existing metric and count
orphaned/non-orphaned x templates.

[smut, 2016-11-23 22:50:57]

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
File appengine/gce-backend/metrics.py (right):

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
appengine/gce-backend/metrics.py:33:
'machine_provider/gce_backend/instances/v2',
On 2016/11/23 02:28:25, Sergey Berezin (google) wrote:
> I'd name it in a more self-descriptive way, e.g. .../instances_by_template.
> Or, even better, just add a new field to the existing metric and count
> orphaned/non-orphaned x templates.

I tried modifying the existing metric and the metric stopped showing up at all.
I thought you once said that field schema shouldn't be changed?

At any rate, the orphaned/unorphaned distinction is somewhat meaningless. An
implementation detail causes tons of orphaned instances to appear in the minutes
following instance deletion. Originally I was counting orphans to find errors,
but now it's standard operating procedure to detect orphans.

[Sergey Berezin (google), 2016-11-23 23:42:44]

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
File appengine/gce-backend/metrics.py (right):

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
appengine/gce-backend/metrics.py:33:
'machine_provider/gce_backend/instances/v2',
On 2016/11/23 at 22:50:57, smut wrote:
> On 2016/11/23 02:28:25, Sergey Berezin (google) wrote:
> > I'd name it in a more self-descriptive way, e.g. .../instances_by_template.
> > Or, even better, just add a new field to the existing metric and count
> > orphaned/non-orphaned x templates.
> 
> I tried modifying the existing metric and the metric stopped showing up at
all. I thought you once said that field schema shouldn't be changed?

No, the field schema should be consistent globally - at least that was a
requirement previously, and apparently now missing fields don't (always) cause
the metric to fail. I'm pretty sure it's OK to change the fields if you redeploy
all the instances that send the metric. Some data may be lost for the transition
period though.

> 
> At any rate, the orphaned/unorphaned distinction is somewhat meaningless. An
implementation detail causes tons of orphaned instances to appear in the minutes
following instance deletion. Originally I was counting orphans to find errors,
but now it's standard operating procedure to detect orphans.

Do you care about the current history of this metric? If not, I'd say let's
update the metric with the new field, drop the 'orphaned', and once deployed,
we'll delete the metric on backend and it will be recreated in its new form. The
reason is, it's a very good name that otherwise would be waisted and be
confusing to everyone. Best to reuse it if we can. WDYT?

[smut, 2016-11-23 23:51:10]

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
File appengine/gce-backend/metrics.py (right):

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
appengine/gce-backend/metrics.py:33:
'machine_provider/gce_backend/instances/v2',
On 2016/11/23 23:42:44, Sergey Berezin (google) wrote:
> On 2016/11/23 at 22:50:57, smut wrote:
> > On 2016/11/23 02:28:25, Sergey Berezin (google) wrote:
> > > I'd name it in a more self-descriptive way, e.g.
.../instances_by_template.
> > > Or, even better, just add a new field to the existing metric and count
> > > orphaned/non-orphaned x templates.
> > 
> > I tried modifying the existing metric and the metric stopped showing up at
> all. I thought you once said that field schema shouldn't be changed?
> 
> No, the field schema should be consistent globally - at least that was a
> requirement previously, and apparently now missing fields don't (always) cause
> the metric to fail. I'm pretty sure it's OK to change the fields if you
redeploy
> all the instances that send the metric. Some data may be lost for the
transition
> period though.
> 
> > 
> > At any rate, the orphaned/unorphaned distinction is somewhat meaningless. An
> implementation detail causes tons of orphaned instances to appear in the
minutes
> following instance deletion. Originally I was counting orphans to find errors,
> but now it's standard operating procedure to detect orphans.
> 
> Do you care about the current history of this metric? If not, I'd say let's
> update the metric with the new field, drop the 'orphaned', and once deployed,
> we'll delete the metric on backend and it will be recreated in its new form.
The
> reason is, it's a very good name that otherwise would be waisted and be
> confusing to everyone. Best to reuse it if we can. WDYT?

No, I don't care about the history. I would prefer this be the only "instances"
metric. Uploaded new patch, and deployed on staging and prod. Should be safe to
delete the old metric's history now and let the new one populate.

[smut, 2016-11-24 00:18:32]

The CQ bit was checked by smut@google.com

[smut, 2016-11-24 00:18:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@google.com
Link to the patchset: https://codereview.chromium.org/2520873003/#ps20001
(title: "Replace old metric with new")

[commit-bot: I haz the power, 2016-11-24 00:18:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 00:20:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479946712437630,
"parent_rev": "f47d1dcc84b711d1d3289e0ca3147a3be7e1d349", "commit_rev":
"d5d2108ec34c507defdc28af67954f9eafe2e5d4"}

[commit-bot: I haz the power, 2016-11-24 00:20:56]

Description was changed from

==========
Count instances managed by each instance template

BUG=664296
==========

to

==========
Count instances managed by each instance template

BUG=664296

Committed:
https://github.com/luci/luci-py/commit/d5d2108ec34c507defdc28af67954f9eafe2e5d4
==========

[commit-bot: I haz the power, 2016-11-24 00:20:57]

Committed patchset #2 (id:20001) as
https://github.com/luci/luci-py/commit/d5d2108ec34c507defdc28af67954f9eafe2e5d4

[Sergey Berezin (google), 2016-11-24 03:51:47]

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
File appengine/gce-backend/metrics.py (right):

https://codereview.chromium.org/2520873003/diff/1/appengine/gce-backend/metri...
appengine/gce-backend/metrics.py:33:
'machine_provider/gce_backend/instances/v2',
On 2016/11/23 at 23:51:09, smut wrote:
> No, I don't care about the history. I would prefer this be the only
"instances" metric. Uploaded new patch, and deployed on staging and prod. Should
be safe to delete the old metric's history now and let the new one populate.

Great, I deleted the metric - once the update propagates, we'll hopefully see
just one metric field. Thanks!