Create a hawkular client for partial endpoints #13814

moolitayer · 2017-02-08T15:30:38Z

Creating the aggregated client does a health check against all endpoints.
The Hawkular deployed in OpenShift does not include some of those endpoints (e.g inventory)

Extracted from #12773

moolitayer · 2017-02-08T15:31:15Z

@miq-bot add_label providers/containers, bug

moolitayer · 2017-02-08T15:31:36Z

@lucasponce @cben @simon3z could you please review?

lucasponce · 2017-02-08T16:45:24Z

@josejulio @cfcosta Please could you also review this PR ?
@moolitayer is using the hawkular ruby client for the datawarehouse provider and he commented some potential problems with the validation of non existing endpoints in the backend.
OpenShift deployments only uses Metrics/Alerting endpoint.

cfcosta

Just some clarifications and some small questions.

cfcosta · 2017-02-08T19:05:01Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-                                         port,
-                                         authentication_token('default'),
-                                         options[:alerts])
+      @clients ||= {}


Where is the @client being defined then? Is it utilized anywhere else in the class?

I think they use the returned variable and only use @client and the new @clients to save the connection for future calls.
Is that right @moolitayer ?

Right, this is a memoization technique @client is defined only here. On the first call it will create and return the relevant client and from that point on it will return the stored value.

cfcosta · 2017-02-08T19:05:47Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-                                         authentication_token('default'),
-                                         options[:alerts])
+      @clients ||= {}
+      @clients[options[:alerts] ? :alerts : :metrics] ||= self.class.raw_connect(


I don't like this options[:alerts] ? :alerts : :metrics inside the hash accessor, can you extract it into a variable? Would make a lot more sense.

Honestly I'm not a fan of creating variables that are not used.
(not a performance thing, only readability)

If you feel current code is not readable I suggest:

@clients ||= {} @clients[ options[:alerts] ? :alerts : :metrics ] ||= self.class.raw_connect( hostname, port, authentication_token('default'), options[:alerts] )

What do you think?

I think the new alternative is more readable. I was also confused with the old one. WDYT @cfcosta ?

changed this for now

The variable would be used, just very locally:

client_type = options[:alerts] ? :alerts : :metrics @clients[client_type] || = ...

But that highlights the fact there are approximately four representations of client type around here:

::Hawkular::Alerts::AlertsClient vs ::Hawkular::Metrics::Client

:alerts vs :metrics here

raw_connect takes positional boolean [3.5. connect takes named boolean]
Q: can any more endpoints become needed later? E.g. Openshift manager fetch_hawk_inv() creates aggregated client then does .strings.get_data().

calling alerts_client (and maybe metrics_client) instead of connect.

Is it feasible to change raw_connect and connect interface to take symbol (:alerts vs :metrics) instead of booleans?
Then you have less representations, can do simpler options[:type] || :metrics, and might not need {alerts,metrics}_client aliases...

There are only two options so It makes more sense to use a boolean instead of a string with two options (openshift cluster only exposes alerts and metrics, string is a type of metric)

Raw connect and connect have their reasons to exists - interfaces calling connect use a context
and raw_connect requires intimate knowledge of provider parameters. If you take a look at other providers they all look exactly the same. container_provider_mixin does something a little different: it passes the options forward, but I see no point in passing some of the parameters as options and some positionals.

moolitayer · 2017-02-08T20:44:27Z

@cfcosta Thanks for the review
@cfcosta @josejulio please take a look

josejulio · 2017-02-09T05:29:35Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

+      )
+    end
+
+    def alerts_client


Might also be worth to create a metrics_client while you are at it.

Sure why not

cfcosta

LGTM

moolitayer · 2017-02-09T10:27:26Z

@cben @josejulio Please review

cben · 2017-02-09T11:00:22Z

I still dislike the boolean because "alerts false" here doesn't mean "the opposite or alerts" or "don't fetch alerts", it means "the other unrelated thing out of two"...

LGTM on the PR, it fixes a bug and the code is no worse than before.

P.S. if the options hash grows any new parameters in the future, it's easy for the bug to re-appear.
In theory the Right Thing is memoizing on @clients[canonicalized_options.freeze] where canonicalized_options currently means ensuring :alerts is either true or false. Not worth the complexity I suppose, if you think that's a concern just add a comment.

P.P.S. AFAIK a Hawkular::Client currently constructs new RestClient for every request, and a RestClient currently opens new http connection for every request. So the memoization is not really helping(?), just dropping it is also an option.

cfcosta · 2017-02-09T11:04:14Z

@cben +1 to dropping memoization, on a stylistic level it makes the code weird to read, and as you said it does not exactly do anything.

In case we're doing lots of that kind of memoization, we can start using Memoist, which is a nice, elegant way of doing memoization.

And as of Ruby 2.1 (I guess) something like this is possible:

memoize def foo; end

And it would be equivalent to do:

def foo; end
memoize :foo

moolitayer · 2017-02-09T12:53:26Z

Outside the scope of this pr:

changing memoization mechanism
should we do memoization?

I have my thoughts about both of these, but they are both out of scope of this PR

in the scope of this pr:

should we pass a boolean or symbol to self.raw_connect if I understand correctly

They both have good reasons, the difference as I see it is;

option a

class.raw_connect(..., false) # or nil or nothing
class.raw_connect(..., true)


def self.raw_connect(..., alerts=false)
  client_klass = alerts ? ...
  ...

option b

class.raw_connect(..., :alerts)
class.raw_connect(..., :events)


def self.raw_connect(..., type=:alerts)
  client_klass = case type
    when :alerts
      ...
    when :event
      ..
    else
      ..

I prefer option a here

moolitayer · 2017-02-09T12:54:30Z

@cben @cfcosta @josejulio please review

cfcosta · 2017-02-09T13:19:05Z

I prefer option b, because its not default behaviour OR other behaviour, but two different kinds of behaviour being switched by a flag. Going even further, I prefer something like this:

def self.client_for(type)
  case type
  when :metrics
   ::Hawkular::Metrics::Client
  when :alerts
    ::Hawkular::Alerts::AlertsClient
  else
    raise ArgumentError, "Client not found for #{type}"
  end
end

def self.raw_connect(..., type = :alerts)
  client_for(type).new(...)
end

moolitayer · 2017-02-09T14:12:27Z

@cben @josejulio are you happy with @cfcosta's suggestion?

moolitayer · 2017-02-09T15:48:56Z

@cben @josejulio sorry for nagging but I have a ton of other stuff dependant on this

josejulio · 2017-02-09T15:50:11Z

@cfcosta suggestion is good

josejulio · 2017-02-09T16:02:23Z

P.P.S. AFAIK a Hawkular::Client currently constructs new RestClient for every request, and a RestClient currently opens new http connection for every request. So the memoization is not really helping(?), just dropping it is also an option.

That's right, but some clients (e.g. metrics) do check the version to know if use legacy API or not.
alerts doesn't do that.

I guess the whole Hawkular client could benefit from checking it lazily (I suppose this PR wouldn't exist if it was like that).

moolitayer · 2017-02-09T16:06:56Z

Thanks @josejulio
@cben are you fine with that?

cben · 2017-02-09T16:17:12Z

Love @cfcosta's suggestion.

moolitayer · 2017-02-09T17:13:20Z

@cben @josejulio @cfcosta please review

josejulio · 2017-02-09T17:31:06Z

LGTM

simon3z · 2017-02-10T14:04:53Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

@@ -35,21 +35,42 @@ def self.verify_ssl_mode
      OpenSSL::SSL::VERIFY_NONE
    end

+    def self.client_for(type)


@moolitayer why don't you keep this in raw_connect?

simon3z · 2017-02-10T14:06:31Z

@miq-bot assign moolitayer

moolitayer · 2017-02-12T12:49:12Z

@miq-bot assign @simon3z
please review

moolitayer · 2017-02-13T08:57:04Z

@simon3z can we merge this?

simon3z · 2017-02-13T09:07:01Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-                                         port,
-                                         authentication_token('default'),
-                                         options[:alerts])
+      raise ArgumentError, "Client not found for #{type}" unless CLIENT_KLASSES.keys.include?(options[:type])


@moolitayer why do you need this? don't you get it for free from raw_connect?

I was worried about the assignment to @client[:something_unexpected] but I guess such assignment would never happen.
Removing

Also not necessary to do the check twice.

cfcosta · 2017-02-13T11:05:34Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

@@ -11,6 +11,11 @@ class Hawkular::DatawarehouseManager < ManageIQ::Providers::DatawarehouseManager
    DEFAULT_PORT = 80
    default_value_for :port, DEFAULT_PORT

+    CLIENT_KLASSES = {


This forces the constants to be autoloaded before the class itself is loaded, so I'd advise against it. The later we manage to move constant loading the better.

cfcosta · 2017-02-13T11:06:30Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-      client = alerts ? ::Hawkular::Alerts::AlertsClient : ::Hawkular::Metrics::Client
-      client.new(
+    def self.raw_connect(hostname, port, token, type = :alerts)
+      raise ArgumentError, "Client not found for #{type}" unless CLIENT_KLASSES.keys.include?(type)


unless CLIENT_CLASSES[type]

Also, no need to name something klass unless it conflicts with a keyword.

cfcosta · 2017-02-13T11:06:49Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

-                                         port,
-                                         authentication_token('default'),
-                                         options[:alerts])
+      raise ArgumentError, "Client not found for #{type}" unless CLIENT_KLASSES.keys.include?(options[:type])


Also not necessary to do the check twice.

cfcosta · 2017-02-13T11:10:01Z

app/models/manageiq/providers/hawkular/datawarehouse_manager.rb

+      )
+    end
+
+    def alerts_client


Since you are already doing that, why not do it with metaprogramming? That way you don't need to add another method in case another client is added.

CLIENT_CLASSES.keys.each do |type| define_method("#{type}_client") do connect(:type => type) end end

miq-bot · 2017-02-13T11:42:21Z

Checked commit moolitayer@aeb4035 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. ⭐

moolitayer · 2017-02-13T17:50:52Z

@simon3z please review

simon3z · 2017-02-13T20:08:51Z

LGTM 👍
cc @chessbyte

@miq-bot assign blomquisg

miq-bot added bug providers/containers labels Feb 8, 2017

moolitayer mentioned this pull request Feb 8, 2017

Event collection for the datawarehouse provider #12773

Merged

chessbyte assigned simon3z Feb 8, 2017

cfcosta reviewed Feb 8, 2017

View reviewed changes

josejulio reviewed Feb 9, 2017

View reviewed changes

moolitayer force-pushed the partial_endpoints branch from 173903c to 0fe66a5 Compare February 9, 2017 09:49

cfcosta approved these changes Feb 9, 2017

View reviewed changes

moolitayer force-pushed the partial_endpoints branch from 0fe66a5 to 24d6f79 Compare February 9, 2017 17:12

moolitayer changed the title ~~Change hawkular client usage to work with partial endpoints~~ Create a hawkular client for partial endpoints Feb 9, 2017

simon3z reviewed Feb 10, 2017

View reviewed changes

miq-bot assigned moolitayer and unassigned simon3z Feb 10, 2017

moolitayer force-pushed the partial_endpoints branch from 24d6f79 to 5998dc6 Compare February 12, 2017 12:36

miq-bot assigned simon3z and unassigned moolitayer Feb 12, 2017

simon3z reviewed Feb 13, 2017

View reviewed changes

cfcosta suggested changes Feb 13, 2017

View reviewed changes

moolitayer force-pushed the partial_endpoints branch from 5998dc6 to 1642239 Compare February 13, 2017 11:38

Create a hawkular client for partial endpoints

aeb4035

moolitayer force-pushed the partial_endpoints branch from 1642239 to aeb4035 Compare February 13, 2017 11:39

miq-bot assigned blomquisg and unassigned simon3z Feb 13, 2017

blomquisg merged commit 4242a1e into ManageIQ:master Feb 14, 2017

blomquisg added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 14, 2017

josejulio mentioned this pull request Feb 23, 2017

Lazy load endpoints. hawkular/hawkular-client-ruby#188

Merged

Create a hawkular client for partial endpoints #13814

Create a hawkular client for partial endpoints #13814

Conversation

moolitayer commented Feb 8, 2017 • edited by miq-bot Loading

moolitayer commented Feb 8, 2017

moolitayer commented Feb 8, 2017

lucasponce commented Feb 8, 2017

cfcosta left a 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

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

moolitayer commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfcosta left a comment

Choose a reason for hiding this comment

moolitayer commented Feb 9, 2017 • edited Loading

cben commented Feb 9, 2017 • edited Loading

cfcosta commented Feb 9, 2017

moolitayer commented Feb 9, 2017 • edited Loading

moolitayer commented Feb 9, 2017

cfcosta commented Feb 9, 2017

moolitayer commented Feb 9, 2017 • edited Loading

moolitayer commented Feb 9, 2017

josejulio commented Feb 9, 2017

josejulio commented Feb 9, 2017

moolitayer commented Feb 9, 2017

cben commented Feb 9, 2017

moolitayer commented Feb 9, 2017

josejulio commented Feb 9, 2017

simon3z Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

simon3z commented Feb 10, 2017

moolitayer commented Feb 12, 2017

moolitayer commented Feb 13, 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

Choose a reason for hiding this comment

miq-bot commented Feb 13, 2017

moolitayer commented Feb 13, 2017

simon3z commented Feb 13, 2017

moolitayer commented Feb 8, 2017 •

edited by miq-bot

Loading

moolitayer commented Feb 9, 2017 •

edited

Loading

cben commented Feb 9, 2017 •

edited

Loading

moolitayer commented Feb 9, 2017 •

edited

Loading

moolitayer commented Feb 9, 2017 •

edited

Loading

simon3z Feb 10, 2017 •

edited

Loading