Prober metrics collection

[Sanger2000]

Added basic architecture for collecting metrics on each probing step.

---

This change is 

[jianglai]

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Sanger2000)

---

prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 70 at r1 (raw file):

C

c

---

prober/src/main/java/google/registry/monitoring/blackbox/messages/EppRequestMessage.java, line 143 at r1 (raw file):

name + " " + expectedResponse.name();

Why do we need the response name?

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 36 at r1 (raw file):

  private static final ImmutableSet<LabelDescriptor> LABELS =

Add a space between each field.

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 39 at r1 (raw file):

t

T

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 45 at r1 (raw file):

by the backend.

by the prober.

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 78 at r1 (raw file):

Quoted 6 lines of code…

  /** Three standard Response types to be recorded as metrics: SUCCESS, FAILURE, or ERROR. */
  public enum ResponseType {
    SUCCESS,
    FAILURE,
    ERROR
  }

Move the definition to the top of the class.

---

prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 120 at r1 (raw file):

  clock = Mockito.mock(Clock.class);

Use a FakeClock instead.

You should than verify that MetricCollector.recordResult is called the the expected arguments. See an example here:

https://sourcegraph.com/github.com/google/nomulus@5797589/-/blob/proxy/src/test/java/google/registry/proxy/handler/BackendMetricsHandlerTest.java#L129

[Sanger2000]

Reviewable status: 9 of 13 files reviewed, 7 unresolved discussions (waiting on @jianglai)

---

prober/src/main/java/google/registry/monitoring/blackbox/ProbingSequence.java, line 70 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

C

c

Done.

---

prober/src/main/java/google/registry/monitoring/blackbox/messages/EppRequestMessage.java, line 143 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

name + " " + expectedResponse.name();

Why do we need the response name?

There are two types of actions we check when we probe "check" for example: "check domainExists" and "check domainNotExists". Similarly, it allows for checking for successful and failing actions and differentiates between them. I.e. the action where we create a domain and expect a success us "create success" but when we expect a failure, it is "create failure".

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 36 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

  private static final ImmutableSet<LabelDescriptor> LABELS =

Add a space between each field.

Done.

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 39 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

t

T

Done.

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 45 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

by the backend.

by the prober.

Done.

---

prober/src/main/java/google/registry/monitoring/blackbox/metrics/MetricsCollector.java, line 78 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

  /** Three standard Response types to be recorded as metrics: SUCCESS, FAILURE, or ERROR. */
  public enum ResponseType {
    SUCCESS,
    FAILURE,
    ERROR
  }

Move the definition to the top of the class.

Done.

---

prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 120 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

  clock = Mockito.mock(Clock.class);

Use a FakeClock instead.

You should than verify that MetricCollector.recordResult is called the the expected arguments. See an example here:

https://sourcegraph.com/github.com/google/nomulus@5797589/-/blob/proxy/src/test/java/google/registry/proxy/handler/BackendMetricsHandlerTest.java#L129

Done.

[jianglai]

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

---

prober/src/main/java/google/registry/monitoring/blackbox/messages/EppRequestMessage.java, line 143 at r1 (raw file):

Previously, Sanger2000 (Aman Sanger) wrote…

There are two types of actions we check when we probe "check" for example: "check domainExists" and "check domainNotExists". Similarly, it allows for checking for successful and failing actions and differentiates between them. I.e. the action where we create a domain and expect a success us "create success" but when we expect a failure, it is "create failure".

It maybe better to break it to two labels. i. e. a label for request name and a label for response name. That way we can easily aggregate if we want to. Say we only care about all checks, we can aggregate all response names.

---

prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 218 at r2 (raw file):

 anyLong()

Is there a way to verify the latency number as well? If you can manually send a response back, you can call FakeClock.advanceOneMilli to advance the clock and verify that you recorded a correct latency.

[Sanger2000]

Reviewable status: 5 of 13 files reviewed, 2 unresolved discussions (waiting on @jianglai)

---

prober/src/main/java/google/registry/monitoring/blackbox/messages/EppRequestMessage.java, line 143 at r1 (raw file):

Previously, jianglai (Lai Jiang) wrote…

It maybe better to break it to two labels. i. e. a label for request name and a label for response name. That way we can easily aggregate if we want to. Say we only care about all checks, we can aggregate all response names.

Done.

---

prober/src/test/java/google/registry/monitoring/blackbox/ProbingSequenceTest.java, line 218 at r2 (raw file):

Previously, jianglai (Lai Jiang) wrote…

 anyLong()

Is there a way to verify the latency number as well? If you can manually send a response back, you can call FakeClock.advanceOneMilli to advance the clock and verify that you recorded a correct latency.

Done.

[jianglai]

Reviewed 8 of 8 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Sanger2000)

---

prober/src/main/java/google/registry/monitoring/blackbox/messages/EppRequestMessage.java, line 147 at r3 (raw file):

  public String responseName() {
    return expectedResponse.name();
  }

Do we still to get the response name from a request? I'd think that by the time you need a response name to record the metrics you must have already got an actual response, right?

[Sanger2000]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jianglai)

---

prober/src/main/java/google/registry/monitoring/blackbox/messages/EppRequestMessage.java, line 147 at r3 (raw file):

Previously, jianglai (Lai Jiang) wrote…

  public String responseName() {
    return expectedResponse.name();
  }

Do we still to get the response name from a request? I'd think that by the time you need a response name to record the metrics you must have already got an actual response, right?

Yeah, but the idea that every request object has an expected response . So, we might have some issue in the channel pipeline beforehand that prevents us from getting that expected response, meaning we wouldn't be able to document the response in that case.

Take for example, an EPP CHECK command that expects back a domainExists. If an issue occurs in the channel pipeline, then we never receive an EppResponseMessage with the requisite name.

We can discuss this more in person, but I don't think it is entirely necessary to get the response object. This could be modified by making the OutboundMessageType point to an InboundMessageType in the future, but again, I think we should discuss this in person.