Introduce: supports :capture #15194

isimluk · 2017-05-23T12:46:10Z

What

Not all providers support metrics :capture. 📊

Currently we cannot tell, so we put perf_capture* into MiqQueue for whatever user has tagged. Putting everything on queue is unfortunate. It kills performance.

Note: This fixes first part of #15193.

Why?

Currently we fail horribly when you tag your scvmm/hyper for C&U capture.

Wait, why did the C&U fail for SCVMM?

It fails at

manageiq/app/models/metric/ci_mixin/capture.rb

Line 3 in 568ff20

self.class.parent::MetricsCapture.new(self)

as self.class.parent::MetricsCapture resolves to Microsoft::Infra::MetricsCapture that does not have any methods for scvmm/hyperv (it's just the base Base::MetricCapture).

durandom · 2017-05-23T13:57:31Z

app/models/metric/ci_mixin.rb

@@ -1,5 +1,6 @@
 module Metric::CiMixin
  extend ActiveSupport::Concern
+  include SupportsFeatureMixin


I think for modules you have to include it like https://github.com/isimluk/manageiq/blob/69901b69264cb03705514e126f8898955b129cc1/app/models/mixins/event_mixin.rb

unless, every module that this mixin includes already has the SupportsFeatureMixin

I see, I got away of this include.

Instead, I made sure (in first commit) that everything that includes CiMixin includes the SupportsFeatureMixin as well.

djberg96 · 2017-05-23T21:01:58Z

I already fought this battle and just resorted to timelines:

#13381

durandom · 2017-05-24T06:41:06Z

@djberg96 good point
Now that @moolitayer did something based on the :metrics feature in ManageIQ/manageiq-providers-kubernetes#7 I think it makes sense to introduce it

durandom

I'm 👍 with it.
@djberg96 @moolitayer any concerns?

isimluk · 2017-05-24T11:48:32Z

@durandom, @lpichler thinks that I could do better than hardcoding scvmm false. So, I come-up with

supports :metrics do
  unless self.class.parent::MetricsCapture.instance_methods.include?(:perf_collect_metrics)
    unsupported_reason_add(:metrics, _('This provider does not support metrics collection'))
  end
end

That works perfect for things that actually run capture.

However, it will not work for ContainerProject, MiqEnterprise, or Zone. That do not capture stuff, they just get metrics_rolups.

So the option is to rename support :metrics to support :capture and have it more fine grained. -> i.e. potentially introduce support :rollups in future if ever needed.

moolitayer · 2017-05-24T12:30:06Z

How do you plan to use these? are the metrics collector workers the ones adding perf_capture* ?
Maybe you can incorporate something like https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/7/files (and then I can remove that code)

The distinction between :capture and :metrics makes sense.

isimluk · 2017-05-24T12:40:39Z

@moolitayer I planned to use it like shown at 43bf24a but not starting the capture worker would be even better, I'll look into it.

miq-bot · 2017-06-01T21:31:39Z

This pull request is not mergeable. Please rebase and repush.

isimluk · 2017-06-05T14:34:51Z

@lpichler It's pity you were so demanding. 😿 Now we have merge conflict and the question that is unanswered for 12 days. Soon this will be closed as a stale pr. :-)

isimluk · 2017-06-05T14:48:28Z

So, I went with the second solution (and that avoids the conflict)

moolitayer · 2017-06-05T15:57:19Z

app/models/container_service.rb

@@ -1,5 +1,6 @@
 class ContainerService < ApplicationRecord
  include CustomAttributeMixin
+  include SupportsFeatureMixin


I'm not sure why this is here?

As far as I know we should collect metrics on container entities if the associated ext has a metrics endpoint

ContainerService include CiMixin that uses SupportsFeatureMixin.

The included do of CiMixin will fail on supports :capture unless the supports means something.

How come I can't see the include like in the other classes? Is that transitive in some way?

https://github.com/isimluk/manageiq/blob/320a55ee0df07a1f2217cb58bd59c4d0e9a3ee15/app/models/container_service.rb#L31

Oh sorry I missed that (I'm on a mobile device)

No problem. I remember I missed that too first time. :-D

miq-bot · 2017-06-06T08:47:49Z

Checked commits isimluk/manageiq@8e7f063~...ec19fba with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 1 offense detected

app/models/mixins/supports_feature_mixin.rb

❗ - Line 92, Col 5 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

moolitayer

LGTM

durandom

Great, thanks @isimluk

@miq-bot assign @agrare

lpichler · 2017-06-07T09:45:26Z

@lpichler It's pity you were so demanding. 😿

@isimluk I am sorry 🐱 I learned it from @durandom 😺

but great, thanks!

djberg96 · 2017-06-07T15:54:59Z

Should those of us who used timelines previously switch to use this?

durandom · 2017-06-07T18:04:33Z

Should those of us who used timelines previously switch to use this?

Not sure. I think metrics, events and timelines are different. Guess this has to be solved from a usage perspective.

isimluk added performance providers/metrics providers/microsoft/infra labels May 23, 2017

isimluk requested a review from durandom May 23, 2017 12:46

isimluk mentioned this pull request May 23, 2017

Do not queue C&U for things that dont support it #15195

Merged

durandom reviewed May 23, 2017

View reviewed changes

chessbyte assigned durandom May 23, 2017

isimluk force-pushed the supports-metrics branch from 69901b6 to a75cf0a Compare May 24, 2017 08:18

durandom approved these changes May 24, 2017

View reviewed changes

miq-bot added the unmergeable label Jun 1, 2017

isimluk force-pushed the supports-metrics branch from a75cf0a to 320a55e Compare June 5, 2017 14:47

miq-bot removed the unmergeable label Jun 5, 2017

moolitayer reviewed Jun 5, 2017

View reviewed changes

isimluk changed the title ~~Introduce: supports :metrics~~ Introduce: supports :capture Jun 6, 2017

isimluk added 2 commits June 6, 2017 10:45

Everything that includes CiMixin shall include SupportsFeature first

8e7f063

Introduce supports feature: metrics: Capacity&Utilization data

ec19fba

isimluk force-pushed the supports-metrics branch from 320a55e to ec19fba Compare June 6, 2017 08:45

moolitayer approved these changes Jun 6, 2017

View reviewed changes

durandom approved these changes Jun 7, 2017

View reviewed changes

agrare merged commit e697fec into ManageIQ:master Jun 7, 2017

agrare added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 7, 2017

isimluk deleted the supports-metrics branch June 7, 2017 14:12

gmcculloug mentioned this pull request Jun 8, 2017

Performance: Do not queue C&U collection for things that do not support C&U #15193

Closed

Fryguy added the providers/infrastructure label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce: supports :capture #15194

Introduce: supports :capture #15194

isimluk commented May 23, 2017 •

edited

Loading

durandom May 23, 2017

durandom May 23, 2017

isimluk May 24, 2017

djberg96 commented May 23, 2017

durandom commented May 24, 2017

durandom left a comment

isimluk commented May 24, 2017 •

edited

Loading

moolitayer commented May 24, 2017

isimluk commented May 24, 2017

miq-bot commented Jun 1, 2017

isimluk commented Jun 5, 2017

isimluk commented Jun 5, 2017

moolitayer Jun 5, 2017

isimluk Jun 5, 2017

moolitayer Jun 6, 2017

isimluk Jun 6, 2017

moolitayer Jun 6, 2017

isimluk Jun 6, 2017

miq-bot commented Jun 6, 2017

moolitayer left a comment

durandom left a comment

lpichler commented Jun 7, 2017 •

edited

Loading

djberg96 commented Jun 7, 2017

durandom commented Jun 7, 2017

Introduce: supports :capture #15194

Introduce: supports :capture #15194

Conversation

isimluk commented May 23, 2017 • edited Loading

What

Why?

Wait, why did the C&U fail for SCVMM?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djberg96 commented May 23, 2017

durandom commented May 24, 2017

durandom left a comment

Choose a reason for hiding this comment

isimluk commented May 24, 2017 • edited Loading

moolitayer commented May 24, 2017

isimluk commented May 24, 2017

miq-bot commented Jun 1, 2017

isimluk commented Jun 5, 2017

isimluk commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 6, 2017

moolitayer left a comment

Choose a reason for hiding this comment

durandom left a comment

Choose a reason for hiding this comment

lpichler commented Jun 7, 2017 • edited Loading

djberg96 commented Jun 7, 2017

durandom commented Jun 7, 2017

isimluk commented May 23, 2017 •

edited

Loading

isimluk commented May 24, 2017 •

edited

Loading

lpichler commented Jun 7, 2017 •

edited

Loading