Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a metric for EPP processing time regardless of ID/TLD #163

Merged
merged 6 commits into from Jul 11, 2019

Conversation

Projects
None yet
4 participants
@gbrodman
Copy link
Collaborator

commented Jul 10, 2019

Assuming this all goes well, we can remove the by-tld and by-id metrics since they're taking up a significantly larger amount of space / cost.


This change is Reviewable

@googlebot googlebot added the cla: yes label Jul 10, 2019

@jianglai
Copy link
Member

left a comment

Are you thinking about getting rid of /epp/processing_time_by_tld? We need to keep it till the open source prober is working because the Spinnaker canary judge uses this metric. This is the only way we can differentiate canary traffic from production traffic, as the probers use different TLDs (iq-any.test vs iq-canary.test, for example) for canary vs prod.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/flows/EppMetrics.java, line 84 at r1 (raw file):

      MetricRegistryImpl.getDefault()
          .newEventMetric(
              "/epp/total_processing_time",

I don't love this ... but unfortunately /epp/processing_time is already in use. Any other ideas?

@gbrodman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 10, 2019

Lai, do we the processing_time (that differentiates by registrar client ID) in the canary judge?

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)


core/src/main/java/google/registry/flows/EppMetrics.java, line 84 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I don't love this ... but unfortunately /epp/processing_time is already in use. Any other ideas?

Maybe request_time?

@jianglai
Copy link
Member

left a comment

That's a good point! If we have the client ID as a label we can use processing_time to differentiate canary from control.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)

@gbrodman
Copy link
Collaborator Author

left a comment

OK so we don't use that already? If we have to keep either the client-ID or TLD differentiators around, I'd much rather keep the TLD differentiator because it's about 1/6 the size.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)

@jianglai
Copy link
Member

left a comment

Right now we use TLD. We need to keep one of them for now till the metrics are recorded by the prober.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)

@gbrodman
Copy link
Collaborator Author

left a comment

OK cool, we will keep TLD around for now.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/flows/EppMetrics.java, line 84 at r1 (raw file):

Previously, gbrodman wrote…

Maybe request_time?

Yeah, I guess that's fine. /epp/request_time works.

@CydeWeys
Copy link
Member

left a comment

It occurs to me that, based on this discussion, we should design the new metric in such a way that canary metrics can be distinguished from prod metrics. That way we can use it for deployments rather than having to keep the by TLD metric around.

What if we add another field to it, call it "frontend" or similar, that will have two values for now, "canary" or "production"? When recording the metric, the value recorded here would be as simple as passing in tld.endsWith("-canary.test") ? "canary" : "production".

We might consider a third value too, for non-canary prober TLDs, e.g.:

If it ends in .canary.test, it gets "canary". Otherwise, if it ends in ".test", it gets "prober". Otherwise, it gets "production'.

Thoughts?

Reviewable status: 0 of 1 files reviewed, all discussions resolved

@CydeWeys
Copy link
Member

left a comment

Maybe "traffic type" makes more sense as a way to describe the three value situation, in which the three values would then be "canary", "prober", and "real".

Reviewable status: 0 of 1 files reviewed, all discussions resolved

@gbrodman

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

Interesting idea. The logic could basically be something like:

if (tld.contains("canary")) return TrafficType.CANARY;
else if (tld.contains("test")) return TrafficType.PROBER;
return TrafficType.REAL;

The order would be important since prober requests also use the .test TLD but that's fine.

gbrodman added some commits Jul 11, 2019

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/flows/EppMetrics.java, line 86 at r3 (raw file):

      MetricRegistryImpl.getDefault()
          .newEventMetric(
              "/epp/processing_time_by_traffic_type",

I don't think the "by_traffic_type" qualifier is particularly necessary. It's also the processing time by command and return status as well, but those don't get called out in the metric despite being at least of equal importance.

I think the former /epp/request_time is better. Once it's the only one of these metrics remaining it'll be a nice, short name. /epp/processing_time_by_traffic_type in isolation isn't a great name, especially since traffic type is just one of three dimensions to it.

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)


core/src/main/java/google/registry/flows/EppMetrics.java, line 86 at r3 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I don't think the "by_traffic_type" qualifier is particularly necessary. It's also the processing time by command and return status as well, but those don't get called out in the metric despite being at least of equal importance.

I think the former /epp/request_time is better. Once it's the only one of these metrics remaining it'll be a nice, short name. /epp/processing_time_by_traffic_type in isolation isn't a great name, especially since traffic type is just one of three dimensions to it.

Done.

@CydeWeys
Copy link
Member

left a comment

:lgtm:

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


core/src/main/java/google/registry/flows/EppMetrics.java, line 46 at r4 (raw file):

          LabelDescriptor.create("command", "The name of the command."),
          LabelDescriptor.create("traffic_type",
              "The traffic type of the command, one of CANARY, PROBER, or REAL."),

Turn the first comma into a semicolon.


core/src/main/java/google/registry/flows/EppMetrics.java, line 93 at r4 (raw file):

  private enum TrafficType {
    CANARY, PROBER, REAL

The Google Java style guide might require these to be on separate lines. I'd check that (and see what google-java-format does too).

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)


core/src/main/java/google/registry/flows/EppMetrics.java, line 46 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Turn the first comma into a semicolon.

I don't think that's strictly necessary since these are fragments but sure, I could see that working due to the commas in the second half


core/src/main/java/google/registry/flows/EppMetrics.java, line 93 at r4 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The Google Java style guide might require these to be on separate lines. I'd check that (and see what google-java-format does too).

The style guide and GJF say that inlined values are fine for very simple enums like this where one-liners are possible

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)


core/src/main/java/google/registry/flows/EppMetrics.java, line 93 at r4 (raw file):

Previously, gbrodman wrote…

The style guide and GJF say that inlined values are fine for very simple enums like this where one-liners are possible

Yay.

@gbrodman gbrodman merged commit 77590dc into google:master Jul 11, 2019

3 of 4 checks passed

code-review/reviewable 1 file, 1 discussion left (CydeWeys)
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kokoro Kokoro build finished
Details

@gbrodman gbrodman deleted the gbrodman:metrics branch Jul 11, 2019

CydeWeys added a commit to CydeWeys/nomulus that referenced this pull request Jul 19, 2019

Add a metric for EPP processing time regardless of ID/TLD (google#163)
* Add a metric for EPP processing time regardless of ID/TLD

* Change name to request_time

* Record EPP processing time by traffic type

* grammar

* request type

* semicolon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.