-
Notifications
You must be signed in to change notification settings - Fork 295
Add GetServiceState action for MoSAPI service monitoring #2906
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
base: master
Are you sure you want to change the base?
Add GetServiceState action for MoSAPI service monitoring #2906
Conversation
|
Gentle ping. |
Implements the `/api/mosapi/getServiceState` endpoint to retrieve service health summaries for TLDs from the MoSAPI system. - Introduces `GetServiceStateAction` to fetch TLD service status. - Implements `MosApiStateService` to transform raw MoSAPI responses into a curated `ServiceStateSummary`. - Uses concurrent processing with a fixed thread pool to fetch states for all configured TLDs efficiently while respecting MoSAPI rate limits. junit test added
da6fc34 to
e79fc94
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 25 files reviewed, 1 unresolved discussion (waiting on @gbrodman and @njshah301).
core/src/test/java/google/registry/mosapi/model/ServiceStateSummaryTest.java line 78 at r2 (raw file):
void testJsonDeserialization() { String json = "{"
Use the Java multi-line string format here and elsewhere. It will also make the quote mark escaping less painful.
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 4 comments.
Reviewable status: 0 of 25 files reviewed, 5 unresolved discussions (waiting on @gbrodman and @njshah301).
core/src/main/java/google/registry/mosapi/GetServiceStateAction.java line 35 at r2 (raw file):
auth = Auth.AUTH_ADMIN) public class GetServiceStateAction implements Runnable { private static final FluentLogger logger = FluentLogger.forEnclosingClass();
More nits, but: generally class declarations look nicer with a blank line after them, to give proper separation to the variables.
core/src/main/java/google/registry/mosapi/GetServiceStateAction.java line 64 at r2 (raw file):
response.setPayload(gson.toJson(stateService.getServiceStateSummary(tld.get()))); } else { response.setPayload(gson.toJson(stateService.getAllServiceStateSummaries()));
getAllServiceStateSummaries() is defined with the following catch block in it:
} catch (MosApiException e) {
logger.atWarning().withCause(e).log(
"Failed to get service state for TLD %s.", tld);
// we don't want to throw exception if fetch failed
return new ServiceStateSummary(tld, FETCH_ERROR_STATUS, null);
}
but then here a few lines below you are also catching the MosApiException and logging it again? Seems like the same problem is going to be logged multiple times in different places? Or are these different errors? It's kind of hard to tell what exceptions are being handled where because everything is just a generic MosApiException.
core/src/main/java/google/registry/config/RegistryConfigSettings.java line 275 at r2 (raw file):
public List<String> tlds; public List<String> services; public Integer tldThreadCnt;
Why Integer instead of int as elsewhere in this file?
core/src/main/java/google/registry/config/files/default-config.yaml line 645 at r2 (raw file):
- "dnssec" #Provides a fixed thread pool for parallel TLD processing.
Missing space after pound sign.
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 25 files reviewed, 6 unresolved discussions (waiting on @gbrodman and @njshah301).
core/src/main/java/google/registry/mosapi/model/IncidentSummary.java line 27 at r2 (raw file):
* 5.1</a> */ public final class IncidentSummary {
Should this perhaps be a record type, or will that not work with Gson annotations? Same comment on below classes as well.
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 25 files reviewed, 7 unresolved discussions (waiting on @gbrodman and @njshah301).
core/src/main/java/google/registry/mosapi/MosApiStateService.java line 55 at r2 (raw file):
/** Shared internal logic to fetch raw data from ICANN MoSAPI state monitoring. */ private TldServiceState fetchRawState(String tld) throws MosApiException {
Are these one line pass-through private functions really necessary? It doesn't seem like they're doing much at all.
njshah301
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@njshah301 made 7 comments and resolved 1 discussion.
Reviewable status: 0 of 25 files reviewed, 6 unresolved discussions (waiting on @CydeWeys and @gbrodman).
core/src/main/java/google/registry/config/RegistryConfigSettings.java line 275 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Why
Integerinstead ofintas elsewhere in this file?
yes, will keep int for consistency
core/src/main/java/google/registry/config/files/default-config.yaml line 645 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Missing space after pound sign.
done
core/src/main/java/google/registry/mosapi/GetServiceStateAction.java line 35 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
More nits, but: generally class declarations look nicer with a blank line after them, to give proper separation to the variables.
done
core/src/main/java/google/registry/mosapi/GetServiceStateAction.java line 64 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
getAllServiceStateSummaries()is defined with the following catch block in it:} catch (MosApiException e) { logger.atWarning().withCause(e).log( "Failed to get service state for TLD %s.", tld); // we don't want to throw exception if fetch failed return new ServiceStateSummary(tld, FETCH_ERROR_STATUS, null); }but then here a few lines below you are also catching the MosApiException and logging it again? Seems like the same problem is going to be logged multiple times in different places? Or are these different errors? It's kind of hard to tell what exceptions are being handled where because everything is just a generic
MosApiException.
we don't want to log as its already logged
core/src/main/java/google/registry/mosapi/MosApiStateService.java line 55 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Are these one line pass-through private functions really necessary? It doesn't seem like they're doing much at all.
we will be using this in triggerAction api also, so that's why kept it as seperate function
core/src/main/java/google/registry/mosapi/model/IncidentSummary.java line 27 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Should this perhaps be a
recordtype, or will that not work with Gson annotations? Same comment on below classes as well.
record type also works, changed to record type to reduce some boilerplate code
core/src/test/java/google/registry/mosapi/model/ServiceStateSummaryTest.java line 78 at r2 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Use the Java multi-line string format here and elsewhere. It will also make the quote mark escaping less painful.
done
17b4c1e to
8c7002d
Compare
- Convert model classes to Java records for conciseness and immutability. - Update unit tests to use Java text blocks for improved JSON readability. - Simplify service and action layers by removing redundant logic and logging. - Fix configuration nits regarding primitive types and comment formatting.
8c7002d to
198e832
Compare
njshah301
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved all comments
@njshah301 made 1 comment.
Reviewable status: 0 of 25 files reviewed, 6 unresolved discussions (waiting on @CydeWeys, @gbrodman, and @ptkach).
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 6 comments and resolved 4 discussions.
Reviewable status: 0 of 25 files reviewed, 8 unresolved discussions (waiting on @gbrodman, @njshah301, and @ptkach).
core/src/main/java/google/registry/mosapi/model/AllServicesStateResponse.java line 29 at r3 (raw file):
* 5.1</a> */ public record AllServicesStateResponse(
These five records are all pretty small .. any reason they need to all be declared in separate files rather than as static inline records on a single file? See e.g. https://cs.opensource.google/nomulus/nomulus/+/master:core/src/main/java/google/registry/bsa/BsaDiffCreator.java;l=243-259;drc=7149fd33071324e4821df1bb90cb4111c4b72816
core/src/main/java/google/registry/mosapi/model/AllServicesStateResponse.java line 31 at r3 (raw file):
public record AllServicesStateResponse( // A list of state summaries for each monitored service (e.g. DNS, RDDS, etc.) @Expose @SerializedName("serviceStates") List<ServiceStateSummary> serviceStates) {
Can this be an ImmutableList? Also why is it nullable? Can you initialize it to empty list?
core/src/main/java/google/registry/mosapi/model/ServiceStatus.java line 33 at r3 (raw file):
// affecting the threshold. @Expose @SerializedName("emergencyThreshold") double emergencyThreshold, @Expose @SerializedName("incidents") List<IncidentSummary> incidents) {
Can this be an ImmutableList? Also why is it nullable? Can you initialize it to empty list?
core/src/test/java/google/registry/mosapi/model/ServiceStatusTest.java line 59 at r3 (raw file):
.contains( """ "status":"Down"\
I wish Java had a better way to do a string without having to escape quotes ...
core/src/main/java/google/registry/mosapi/model/ServiceStateSummary.java line 34 at r3 (raw file):
@Expose @SerializedName("tld") String tld, @Expose @SerializedName("overallStatus") String overallStatus, @Expose @SerializedName("activeIncidents") @Nullable List<ServiceStatus> activeIncidents) {
Can this be an ImmutableList? Also why is it nullable? Can you initialize it to empty list?
core/src/main/java/google/registry/mosapi/model/TldServiceState.java line 37 at r3 (raw file):
// DNS, // RDDS, EPP, DNSSEC, RDAP). @Expose @SerializedName("testedServices") Map<String, ServiceStatus> serviceStatuses) {
Can this be an ImmutableMap? Also why is it nullable? Can you initialize it to empty map?
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 25 files reviewed, 7 unresolved discussions (waiting on @gbrodman, @njshah301, and @ptkach).
core/src/main/java/google/registry/mosapi/MosApiStateService.java line 55 at r2 (raw file):
Previously, njshah301 (Nilay Shah) wrote…
we will be using this in triggerAction api also, so that's why kept it as seperate function
Why can't the trigger action API just call serviceMonitoringClient.getTldServiceState(tld) itself?
Implements the
/api/mosapi/getServiceStateendpoint to retrieve service health summaries for TLDs from the MoSAPI system.GetServiceStateActionto fetch TLD service status.MosApiStateServiceto transform raw MoSAPI responses into a curatedServiceStateSummary.junit test added
This change is