Add the ability to configure a ratio of proxy metrics to be recorded

[CydeWeys]

This ratio defaults to 1.0 (i.e. all metrics will be recorded), but we will set it much lower in sandbox and production, probably something closer to 0.01. This will reduce recorded metrics volume and thus StackDriver cost, while still retaining enough data for overall performance monitoring.

This is handled stochastically, so as to not require any coordination between Java threads or GKE pods/clusters, as alternative approaches would (i.e. using a counter and recording every Nth, or throttling to a max metrics qps).

BUG = http://b/364907042

---

This change is 

[CydeWeys]

Still a WIP; I need to get the tests working (by creating dummy proxy config objects for them).

[gbrodman]

lgtm so far but it needs test changes

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @CydeWeys)

[gbrodman]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 85 at r2 (raw file):

  private static final Random random = new Random();

  double frontendMetricsRatio;

private?

---

proxy/src/main/java/google/registry/proxy/metric/BackendMetrics.java line 82 at r2 (raw file):

  private static final Random random = new Random();

  double backendMetricsRatio;

private?

[CydeWeys]

PTAL, now with passing tests.

Reviewable status: 4 of 8 files reviewed, 2 unresolved discussions (waiting on @gbrodman)

---

proxy/src/main/java/google/registry/proxy/metric/BackendMetrics.java line 82 at r2 (raw file):

Previously, gbrodman wrote…

private?

Can't be.

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 85 at r2 (raw file):

Previously, gbrodman wrote…

private?

Can't be.

[gbrodman]

Reviewed all commit messages.
Reviewable status: 4 of 8 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)

---

proxy/src/main/java/google/registry/proxy/metric/BackendMetrics.java line 82 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Can't be.

now that the tests are added, yeah

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 85 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Can't be.

now that the tests are added, yeah

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

  @NonFinalForTesting
  @VisibleForTesting
  Random random = new Random();

if you inject this, it doesn't need to be non-final-for-testing

[gbrodman]

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

[CydeWeys]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gbrodman)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

Previously, gbrodman wrote…

if you inject this, it doesn't need to be non-final-for-testing

I don't know if we need to @Inject Random though; we don't do so anywhere else in our codebase. Adding dependency injection solely to modify it in a test seems unnecessary.

[gbrodman]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

we don't do so anywhere else in our codebase

https://github.com/google/nomulus/blob/master/core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java#L98 ?

ether way, it's probably the better pattern anyway

[CydeWeys]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gbrodman)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

Previously, gbrodman wrote…

we don't do so anywhere else in our codebase

https://github.com/google/nomulus/blob/master/core/src/main/java/google/registry/tools/server/RefreshDnsForAllDomainsAction.java#L98 ?

ether way, it's probably the better pattern anyway

What's probably the better pattern?

[gbrodman]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What's probably the better pattern?

injection -- plus, we inject SecureRandoms all over the place anyway so it's not unique

[CydeWeys]

Reviewable status: 2 of 8 files reviewed, 3 unresolved discussions (waiting on @gbrodman)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

Previously, gbrodman wrote…

injection -- plus, we inject SecureRandoms all over the place anyway so it's not unique

Done.

[CydeWeys]

PTAL

Reviewable status: 2 of 8 files reviewed, 3 unresolved discussions (waiting on @gbrodman)

[CydeWeys]

PTAL

Reviewable status: 2 of 8 files reviewed, 3 unresolved discussions (waiting on @gbrodman)

[gbrodman]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CydeWeys)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Done.

now these can both be private right?

---

proxy/src/test/java/google/registry/proxy/metric/FrontendMetricsTest.java line 37 at r5 (raw file):

  private static final String PROTOCOL = "some protocol";
  private static final String CERT_HASH = "abc_blah_1134zdf";
  private final FrontendMetrics metrics = new FrontendMetrics(1.0, new Random());

generally the pattern we use is mocking the random and creating the object in the beforeEach method

---

proxy/src/main/java/google/registry/proxy/ProxyModule.java line 415 at r5 (raw file):

  @Singleton
  @Provides
  static Random provideRandom() {

is it worth doing this vs just using the SecureRandom used / injected elsewhere ?

[CydeWeys]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gbrodman)

---

proxy/src/main/java/google/registry/proxy/ProxyModule.java line 415 at r5 (raw file):

Previously, gbrodman wrote…

is it worth doing this vs just using the SecureRandom used / injected elsewhere ?

SecureRandom is much more resource-intensive and could possibly even become a bottleneck when QPS starts hitting four digits. It's not worth it.

---

proxy/src/test/java/google/registry/proxy/metric/FrontendMetricsTest.java line 37 at r5 (raw file):

Previously, gbrodman wrote…

generally the pattern we use is mocking the random and creating the object in the beforeEach method

Not worth doing here, as it only needs to be overridden for one test. Overriding it for every test doesn't make sense, as there's no common set of return values that would work for all tests.

[CydeWeys]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gbrodman)

---

proxy/src/main/java/google/registry/proxy/metric/FrontendMetrics.java line 86 at r3 (raw file):

Previously, gbrodman wrote…

now these can both be private right?

No, because they're being overridden in tests.

[gbrodman]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys)

---

proxy/src/main/java/google/registry/proxy/ProxyModule.java line 415 at r5 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

SecureRandom is much more resource-intensive and could possibly even become a bottleneck when QPS starts hitting four digits. It's not worth it.

oh interesting, good to know

---

proxy/src/test/java/google/registry/proxy/metric/FrontendMetricsTest.java line 37 at r5 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Not worth doing here, as it only needs to be overridden for one test. Overriding it for every test doesn't make sense, as there's no common set of return values that would work for all tests.

what i see in other tests, what i think tends to be the cleanest, is to create the default state of the object (with 1.0 and a real Random) in the BeforeEach method and then create a new object in each test method where the default state is not valid (say, with a different threshold or a mocked Random)

or we just have the action be only a local variable each test case to be explicit (since that wouldn't take any longer than creating it as an instance variable here).

if we're not going to do that, then I think the initial approach (non-final-for-testing variable) was probably better

[CydeWeys]

PTAAL

Reviewable status: 4 of 8 files reviewed, 3 unresolved discussions (waiting on @gbrodman)

---

proxy/src/test/java/google/registry/proxy/metric/FrontendMetricsTest.java line 37 at r5 (raw file):

Previously, gbrodman wrote…

what i see in other tests, what i think tends to be the cleanest, is to create the default state of the object (with 1.0 and a real Random) in the BeforeEach method and then create a new object in each test method where the default state is not valid (say, with a different threshold or a mocked Random)

or we just have the action be only a local variable each test case to be explicit (since that wouldn't take any longer than creating it as an instance variable here).

if we're not going to do that, then I think the initial approach (non-final-for-testing variable) was probably better

Done.

[gbrodman]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

[CydeWeys]

PTAL, needs approval re-upped because I had to format a file.

Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @gbrodman)

[gbrodman]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys)