Feat add metrics #256

Inksprout · 2022-01-24T23:42:33Z

The purpose of this PR is to add code that can be used to push metrics from pact-ruby to google analytics.
The PR adds event tracking for when pact verification has completed. The code supports users turning off this tracking by setting the env var PACT_DO_NOT_TRACK to 'true'.

lib/pact/utils/metrics.rb

lib/pact/provider/pact_spec_runner.rb

Co-authored-by: Matt Fellows <matt.fellows@onegeek.com.au>

spec/pact/utils/metrics_spec.rb

lib/pact/utils/metrics.rb

bethesque · 2022-01-27T00:16:23Z

lib/pact/utils/metrics.rb

+
+        if track_events?
+          Pact.configuration.output_stream.puts "WARN: Please note: we are tracking events anonymously to gather important usage statistics like Pact-Ruby version
+            and operating system. To disable tracking, set the 'pact_do_not_track' environment


Upper case PACT_DO_NOT_TRACK.

The other implementations have followed the original Pact.js implementation, which uses a lower case environment variable name

Oh, I think you've misread the code. In Pact Node, you can set the pact_do_not_track config item (i.e. npm config set pact_do_not_track true or in package.json).

We still support the env var PACT_DO_NOT_TRACK also, see https://github.com/pact-foundation/pact-js-core/blob/pact-node/standalone/install.ts#L85.

Ah, in that case we need to change the other implementations. Probably best to make it case-insensitive

So, for clarity, Upper case PACT_DO_NOT_TRACK is what we want for consistency (including the warning). We can update rust later.

bethesque · 2022-01-27T00:17:22Z

lib/pact/utils/metrics.rb

+            "av" => Pact::VERSION,
+            "aid" => "pact-ruby",
+            "aip" => 1,
+            "ds" => ENV["CI"] || "unknown",


ENV["CI"] will eq 'true'. You need this to return "ci" if ENV["CI"] == 'true'.

bethesque · 2022-01-27T00:17:59Z

lib/pact/utils/metrics.rb

+            "aid" => "pact-ruby",
+            "aip" => 1,
+            "ds" => ENV["CI"] || "unknown",
+            "cd2" => ENV['PACT_EXECUTING_LANGUAGE'] ? "client" : "unknown",


What is cd2? I think this will always be 'client'.

Ah I think what's happened is I have mixed up the 'ds' field and 'cd2' field.
ds should be one of: client, cli, broker
while cd2 should be "One of: CI, development, unknown"
For cd2 Ron suggested it should be "CI" if the CI env var is set or otherwise "unknown". If that sounds good I'll fix up these 2 fields

bethesque · 2022-01-27T00:21:36Z

lib/pact/utils/metrics.rb

+            "cd2" => ENV['PACT_EXECUTING_LANGUAGE'] ? "client" : "unknown",
+            "cd3" => RUBY_PLATFORM,
+            "cd6" => ENV['PACT_EXECUTING_LANGUAGE'] || "unknown",
+            "cd7" => RUBY_VERSION,


I think we only want to use the ruby version if the executing language is Ruby.

when it's not ruby would we just leave this blank?

Should get it from the wrapping language implementation

So perhaps ENV['PACT_EXECUTING_LANGUAGE_VERSION']?

Thats seems sensible to me!

bethesque · 2022-01-27T23:04:18Z

The next thing we need to do is make this call interrupt the user as little as possible. We want to keep the connection and request timeouts very small.

Some reading:
https://felipeelias.github.io/ruby/2017/08/20/net-http-timeouts.html
https://ruby-doc.org/stdlib-2.2.4/libdoc/net/http/rdoc/Net/HTTP.html (search for timeout in here)

Do you think it's worth using a thread to make sure it's non blocking @uglyog? How have you implemented it in other languages?

bethesque · 2022-01-27T23:04:50Z

We also need to make sure any exception that gets thrown does not impact or get shown to the user.

uglyog · 2022-01-27T23:07:29Z

I've implemented it as a background thread in other places

bethesque · 2022-01-27T23:12:51Z

Cool. This looks straightforward https://www.rubyguides.com/2015/07/ruby-threads/

bethesque · 2022-02-01T06:18:05Z

Unlike the mock service, this one is reporting at the end of the event, and it's possible that the ruby process will have shut down before the http request is completed. I think we should either call it before the test is run, so it can have lots of time to make the http call, or we need to do a thread.join to ensure it's finished before the tests shut down.

TimothyJones · 2022-02-02T02:34:30Z

As mentioned here, I think we should not implement this change.

Tracking the versions of runtime is potentially information an attacker can use
This updates pact to call home rather than just track the downloads
Our tracking is opt-out by default (in pact-js have a friendly message that essentially says "we just tracked you! Let us know if you want us to not do that again")

If we do bring in a change like this, I think it should be opt-in by default - as that's the trend (and in my view best-behaviour) these days anyway. I created pact-foundation/pact-js-core#360 to track this for pact-js.

Critically, I think the GDPR is relevant here, too - as it means that we're gaining access to information that is already stored on the user's machine that could be used in the process of identifying them (and isn't required for the operation of the service, which means the exception doesn't apply). This means it must be opt-in. There's some discussion on stack exchange here (although it's about local storage, the discussion is relevant).

mefellows · 2022-02-02T03:04:33Z

@TimothyJones I understand your concern, rather than duplicating responses, I'll refer to https://github.com/pact-foundation/pact-js-core/pull/359#issuecomment-1027529629] which sums up the thinking so future readers will know where to look.

mefellows · 2022-02-09T23:31:29Z

Confirming the telemetry docs page is now live and shared with maintainers, who have given it the 👍 .

bethesque · 2022-02-21T01:19:39Z

lib/pact/utils/metrics.rb

+
+      def self.handle_error e
+      if ENV['PACT_METRICS_DEBUG'] == 'true'
+        Pact.configuration.output_stream.puts("DEBUG: #{e.inspect}")


e.inspect doesn't include the backtrace. Can you make this

Pact.configuration.output_stream.puts("DEBUG: #{e.inspect}\n" + e.backtrace.join("\n"))

bethesque · 2022-02-21T01:19:59Z

lib/pact/utils/metrics.rb

+
+      private
+
+      def self.handle_error e


Indentation is a bit off here.

The upgrade to pact 1.62.0 includes pact-foundation/pact-ruby#256 which adds telemetry[[1]] for the package maintainers. Set the `PACT_DO_NOT_TRACK` environment variable which opts out of sending this telemetry. [1]: https://docs.pact.io/telemetry/

Pact have recently added tracking to their project which will result in data sent from our build servers to their servers [1]. I'm not sure if we have a collective opinion on whether we're happy with this or not, so it seems simplest to disable it and save the need for a divisive decision. [1]: pact-foundation/pact-ruby#256

Pact have recently added tracking to their project which will result in data sent from our build servers to their servers [1]. I'm not sure if we have a collective opinion on whether we're happy with this or not, so it seems simplest to disable it and save the need for a divisive decision. It also gets rid of an annoying warning. [1]: pact-foundation/pact-ruby#256

Inksprout added 2 commits January 24, 2022 17:22

feat: Add metrics util and report metric for pact verification

284d4c1

fix: minor clean up

925c888

uglyog requested changes Jan 24, 2022

View reviewed changes

lib/pact/utils/metrics.rb Outdated Show resolved Hide resolved

mefellows reviewed Jan 24, 2022

View reviewed changes

lib/pact/provider/pact_spec_runner.rb Outdated Show resolved Hide resolved

Inksprout and others added 2 commits January 25, 2022 10:59

Update metrics.rb

a6f1a74

Update lib/pact/provider/pact_spec_runner.rb

0bdc8ce

Co-authored-by: Matt Fellows <matt.fellows@onegeek.com.au>

mefellows reviewed Jan 25, 2022

View reviewed changes

spec/pact/utils/metrics_spec.rb Outdated Show resolved Hide resolved

mefellows reviewed Jan 25, 2022

View reviewed changes

lib/pact/utils/metrics.rb Outdated Show resolved Hide resolved

mefellows reviewed Jan 25, 2022

View reviewed changes

lib/pact/utils/metrics.rb Outdated Show resolved Hide resolved

fix: PR feedback

191a5ad

Inksprout requested a review from uglyog January 25, 2022 00:18

uglyog approved these changes Jan 25, 2022

View reviewed changes

mefellows approved these changes Jan 25, 2022

View reviewed changes

bethesque requested changes Jan 27, 2022

View reviewed changes

Inksprout added 2 commits January 27, 2022 14:58

fix: PR feedback

22b33bc

fix: update specs

46dc59c

Inksprout added 3 commits January 31, 2022 11:34

fix: add timeout and threading

b12d332

fix: add exception handling and fix rake tasks

69854ac

fix: reduce complexity

47a203f

Inksprout requested a review from bethesque January 31, 2022 07:27

Inksprout mentioned this pull request Feb 2, 2022

feat: collect usage telemetry data pact-foundation/pact-js-core#359

Merged

fix: PR feedback give the http call more time to send

a20796d

bethesque reviewed Feb 21, 2022

View reviewed changes

lib/pact/utils/metrics.rb

private

def self.handle_error e

Copy link

Member

bethesque Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is a bit off here.

Inksprout reacted with thumbs up emoji

fix: PR feedback

b3fa821

Inksprout requested a review from bethesque February 21, 2022 04:51

bethesque merged commit 4497ee9 into pact-foundation:master Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat add metrics #256

Feat add metrics #256

Inksprout commented Jan 24, 2022

bethesque Jan 27, 2022

uglyog Jan 27, 2022

mefellows Jan 27, 2022 •

edited

Loading

uglyog Jan 27, 2022

mefellows Jan 27, 2022

bethesque Jan 27, 2022

bethesque Jan 27, 2022

Inksprout Jan 27, 2022

bethesque Jan 27, 2022

Inksprout Jan 27, 2022

uglyog Jan 27, 2022

Inksprout Jan 27, 2022

mefellows Jan 27, 2022

bethesque commented Jan 27, 2022

bethesque commented Jan 27, 2022

uglyog commented Jan 27, 2022

bethesque commented Jan 27, 2022

bethesque commented Feb 1, 2022 •

edited

Loading

TimothyJones commented Feb 2, 2022

mefellows commented Feb 2, 2022

mefellows commented Feb 9, 2022

bethesque Feb 21, 2022

bethesque Feb 21, 2022

Feat add metrics #256

Feat add metrics #256

Conversation

Inksprout commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mefellows Jan 27, 2022 • edited Loading

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

Choose a reason for hiding this comment

bethesque commented Jan 27, 2022

bethesque commented Jan 27, 2022

uglyog commented Jan 27, 2022

bethesque commented Jan 27, 2022

bethesque commented Feb 1, 2022 • edited Loading

TimothyJones commented Feb 2, 2022

mefellows commented Feb 2, 2022

mefellows commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mefellows Jan 27, 2022 •

edited

Loading

bethesque commented Feb 1, 2022 •

edited

Loading