(PE-18347) Add metric_id option to http requests in termini by rlinehan · Pull Request #2179 · puppetlabs/puppetdb

rlinehan · 2017-02-28T17:12:31Z

Add the metric_id option to the http requests in the facts terminus,
resource terminus, query function, and for command submission. These
metric_ids will be used by the http client Puppet Server uses to create
metrics in its registry, which can then be queried to determine how long
requests Puppet Server makes to PuppetDB are taking.

For each element in a metric_id array, a metric is created. So, a metric_id
[:foo :bar :baz] will result in metrics foo, foo.bar, and foo.bar.baz.
The elements in the array can be symbols or strings. For more information, see
the documentation on metrics in the clj-http-client library.

For the facts find action, the metric_id is [:puppetdb :facts :find <node-name>].

For the facts search action, the metric_id is [:puppetdb :facts :search]. The query is not included as part of the metric_id because it
would not create useful metrics to capture - each search query can be pretty
much anything, and tracking a single search query over time probably won't be
very useful.

For the resource search action, the metric_id is [:puppetdb :resource :search <resource type>].

For the http call made by the puppetdb query function, the metric_id is
[:puppetdb :query]. This metric-id does not include the actual query, since
there are so many possible queries that tracking metrics on them does not seem
particularly useful.

For the http requests for submitting commands, metric_ids will be
[:puppetdb :command <command name> <certname>].

puppetlabs-jenkins · 2017-02-28T17:39:18Z

Test FAILed

mullr · 2017-02-28T17:47:10Z

@rlinehan For the command metrics, why do you want certname in the metric id? That's the certname of the node being managed, so there will be lots of them.

rlinehan · 2017-02-28T17:51:05Z

@mullr same is true for certname in the facts.find. We track certname also on compilation metrics and some others. Some users may find it useful if there is a specific node that is having problems. I guess it is less likely that the problem will be on the puppetdb side of things, but still seems like it could be potentially useful.

The way the metric ids work (which I realize I forgot to put in the commit message... I can amend it), is that a metric gets created for each element in the array - so for [:puppetdb :command <command name> <node name>] you would get puppetdb, puppetdb.command, puppetdb.command.<command name> and puppetdb.command.<command name>.<node name>.

mullr · 2017-02-28T17:54:26Z

@rlinehan ah, I see. That sounds quite useful.

rlinehan · 2017-02-28T17:55:36Z

Jenkins, retest this please.

puppetcla · 2017-02-28T18:00:25Z

CLA signed by all contributors.

puppetlabs-jenkins · 2017-02-28T18:14:26Z

Test FAILed

Add the `metric_id` option to the http requests in the facts terminus, resource terminus, query function, and for command submission. These `metric_id`s will be used by the http client Puppet Server uses to create metrics in its registry, which can then be queried to determine how long requests Puppet Server makes to PuppetDB are taking. For each element in a `metric_id` array, a metric is created. So, a `metric_id` `[:foo :bar :baz]` will result in metrics `foo`, `foo.bar`, and `foo.bar.baz`. The elements in the array can be symbols or strings. For more information, see the [documentation on metrics in the clj-http-client library](https://github.com/puppetlabs/clj-http-client/blob/master/doc/metrics.md). For the facts `find` action, the `metric_id` is `[:puppetdb :facts :find <node-name>]`. For the facts `search` action, the `metric_id` is `[:puppetdb :facts :search]`. The query is not included as part of the `metric_id` because it would not create useful metrics to capture - each search query can be pretty much anything, and tracking a single search query over time probably won't be very useful. For the resource `search` action, the `metric_id` is `[:puppetdb :resource :search <resource type>]`. For the http call made by the puppetdb query function, the `metric_id` is `[:puppetdb :query]`. This metric-id does not include the actual query, since there are so many possible queries that tracking metrics on them does not seem particularly useful. For the http requests for submitting commands, `metric_id`s will be `[:puppetdb :command <command name> <certname>]`.

rlinehan · 2017-03-01T14:57:14Z

Realized I had missed the resource terminus, so amended my commit to include that and also updated the commit message to describe how metric-ids work.

puppetlabs-jenkins · 2017-03-01T15:47:49Z

Test FAILed

rlinehan · 2017-03-01T18:15:45Z

Hmmm looks like actually I broke things....

rlinehan · 2017-03-02T17:18:54Z

Well, I figured out why I broke things. Locally I'm running this against Puppet Server at HEAD of master, which has https://github.com/puppetlabs/puppetserver/pull/1331/files#diff-5805ba6eb4857e236651f3c3fcdffeb0R75, which changes .get to allow for options to be passed in as a third argument.

So, the question is, do we ever make any requirements about PuppetDB and Puppet Server compatibility? Can we say that if you run PuppetDB 5 you must run Puppet Server 5?

Prior to Puppet Server 5, Puppet Server's http client class had an implementation of `get` with an arity of 2 (where `path` and `headers` are the arguments). However, in Ruby Puppet and in Puppet Server 5, the http client class has an implementation of `get` with an arity of 3 (with arguments `path`, `headers`, and `options`). Since we do not know under which version of Puppet Server or the Ruby Puppet master this terminus code will be run under, this commit adds a `multi_arity_get` helper function that inspects the `http_instance` class to determine the arity of `get` before calling it with the appropriate arguments for its arity.

puppetlabs-jenkins · 2017-03-07T16:07:15Z

Test PASSed

rlinehan · 2017-03-07T16:24:46Z

Heyyyy tests are passing! @camlow325 or @jpinsonault could you take a look at this when you get a chance and make sure this seems reasonable to you?

camlow325 · 2017-03-07T19:45:44Z

+      # `parameters()` returns the parameters of a method as an array of
+      # tuples, with a tuple for each parameter, e.g. `[[:req :a], [:opt
+      # :b]]`. Counting this array gives the arity.
+      arity = http_instance.class.instance_method(:get).parameters.count


Do you know how fast this call is? If this is measurably slow, I wonder if it would be better to memoize the parameter count.

It seems pretty fast - benchmarking with calling it 5000 times memoized and not resulted in

user system total real not: 0.000000 0.000000 0.000000 ( 0.004518) memoized: 0.000000 0.000000 0.000000 ( 0.000412)

so... a small difference, and memoizing it is very easy, so might as well.

camlow325 · 2017-03-07T20:06:11Z

+      # tuples, with a tuple for each parameter, e.g. `[[:req :a], [:opt
+      # :b]]`. Counting this array gives the arity.
+      arity = http_instance.class.instance_method(:get).parameters.count
+      if arity == 2


Could do a case/when here instead, I suppose.

camlow325

This LGTM. I didn't do any functional testing with it but assuming that since it's passing with current Puppet Server code that it isn't breaking anything. The more telling work to prove that it's good would probably be done with SERVER-1745. Just the one question about perf with the multi-arity get() function handling.

senior · 2017-03-09T15:20:16Z

@rlinehan We've not had a requirement for a specific version of Puppet Server to run PuppetDB. Did that part get fixed in this PR or is merging it mean we must have Puppet Server 5? Also has Puppet Server 5 been released yet? I just see 2.7.2-SNAPSHOT in the repo now.

camlow325 · 2017-03-09T15:51:04Z

@senior As this PR currently stands, it should work either with released versions of Puppet Server - 2.7.2 or earlier - as well as work on the Puppet Server master branch which would roll into Puppet Server 5.0. So we shouldn't have to do any version pinning in PuppetDB packaging in conjunction with the work in this PR. We haven't released Puppet Server 5.0 yet. We should change our SNAPSHOT versions on our master branch to reflect that they are building up to 5.0, though. I'll make a note to do that soon...

Memoize the call to get the arity of the `get` method in the http client class, since this call takes more time than just a regular method call. Also update to use a case/when block rather than if/else.

rlinehan · 2017-03-10T02:20:27Z

Pushed up another commit memoizing the call to get the parameter count.

@jb I have done a small amount of functional testing by running PuppetDB with this change locally along with Puppet Server at both an old commit and a newer-commit - including with the status endpoint changes (that I haven't pushed up anywhere). It works at a commit on stable (Puppet Server 2.7.2) and also on my newer changes (will be Puppet Server 5), and reports metrics to the status endpoint.

@senior Yes, with the second commit (adding the multi_arity_get function) this should work with any version of Puppet Server or with just Ruby Puppet. The acceptance tests were failing before because y'all are running tests against Puppet Server 2.7.2. The fact that these tests are now passing (with the new commit) means that the compatibility will not be broken.

puppetlabs-jenkins · 2017-03-10T02:42:48Z

Test FAILed

puppetlabs-jenkins · 2017-03-10T03:30:03Z

Test PASSed

camlow325 · 2017-03-10T22:14:18Z

This looks okay to me to merge. @senior, you good with this?

rlinehan force-pushed the ticket/master/PE-18347-add-metric-ids-in-termini-http-calls branch from bd1961e to a98f40a Compare March 1, 2017 14:56

camlow325 reviewed Mar 7, 2017

View reviewed changes

camlow325 approved these changes Mar 7, 2017

View reviewed changes

(PE-18347) Memoize arity call in multi_arity_get method

6ae215d

Memoize the call to get the arity of the `get` method in the http client class, since this call takes more time than just a regular method call. Also update to use a case/when block rather than if/else.

rlinehan force-pushed the ticket/master/PE-18347-add-metric-ids-in-termini-http-calls branch from 74595e1 to 6ae215d Compare March 10, 2017 02:15

senior approved these changes Mar 13, 2017

View reviewed changes

senior merged commit 6bfbcfd into puppetlabs:master Mar 13, 2017

Uh oh!

Conversation

rlinehan commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puppetlabs-jenkins commented Feb 28, 2017

Uh oh!

mullr commented Feb 28, 2017

Uh oh!

rlinehan commented Feb 28, 2017

Uh oh!

mullr commented Feb 28, 2017

Uh oh!

rlinehan commented Feb 28, 2017

Uh oh!

puppetcla commented Feb 28, 2017

Uh oh!

puppetlabs-jenkins commented Feb 28, 2017

Uh oh!

rlinehan commented Mar 1, 2017

Uh oh!

puppetlabs-jenkins commented Mar 1, 2017

Uh oh!

rlinehan commented Mar 1, 2017

Uh oh!

rlinehan commented Mar 2, 2017

Uh oh!

puppetlabs-jenkins commented Mar 7, 2017

Uh oh!

rlinehan commented Mar 7, 2017

Uh oh!

camlow325 Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

rlinehan Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

camlow325 Mar 7, 2017

Choose a reason for hiding this comment

Uh oh!

camlow325 left a comment

Choose a reason for hiding this comment

Uh oh!

senior commented Mar 9, 2017

Uh oh!

camlow325 commented Mar 9, 2017

Uh oh!

rlinehan commented Mar 10, 2017

Uh oh!

puppetlabs-jenkins commented Mar 10, 2017

Uh oh!

puppetlabs-jenkins commented Mar 10, 2017

Uh oh!

camlow325 commented Mar 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rlinehan commented Feb 28, 2017 •

edited

Loading