SDT-159: SDT: submitQuery response status doesn't reflect results ret… #464
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Plan Result
Change Result (Click me) # module.application_insights.azurerm_application_insights.this will be updated in-place
~ resource "azurerm_application_insights" "this" {
~ daily_data_cap_notifications_disabled = false -> true
id = "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/civil-sdt-aat/providers/Microsoft.Insights/components/civil-sdt-appinsights-aat"
name = "civil-sdt-appinsights-aat"
tags = {
"application" = "civil"
"autoShutdown" = "true"
"builtFrom" = "https://github.com/HMCTS/civil-sdt.git"
"businessArea" = "CFT"
"contactSlackChannel" = "#civil_contact"
"environment" = "staging"
"managedBy" = "Civil"
}
# (15 unchanged attributes hidden)
}
# module.application_insights.azurerm_monitor_action_group.action_group will be created
+ resource "azurerm_monitor_action_group" "action_group" {
+ enabled = true
+ id = (known after apply)
+ location = "global"
+ name = "Civil-Aat-Warning-Alerts"
+ resource_group_name = "civil-sdt-aat"
+ short_name = "civ-aat"
+ tags = {
+ "application" = "civil"
+ "autoShutdown" = "true"
+ "builtFrom" = "https://github.com/HMCTS/civil-sdt.git"
+ "businessArea" = "CFT"
+ "contactSlackChannel" = "#civil_contact"
+ "environment" = "staging"
+ "managedBy" = "Civil"
}
+ azure_function_receiver {
+ function_app_resource_id = "/subscriptions/96c274ce-846d-4e48-89a7-d528432298a7/resourceGroups/cft-alerts-slack-aat/providers/Microsoft.Web/sites/cft-alerts-slack-aat"
+ function_name = "httpTrigger"
+ http_trigger_url = (sensitive value)
+ name = "slack-alerts"
+ use_common_alert_schema = true
}
}
# module.application_insights.azurerm_monitor_activity_log_alert.main will be created
+ resource "azurerm_monitor_activity_log_alert" "main" {
+ description = "Monitors for application insight reaching it's daily cap."
+ enabled = true
+ id = (known after apply)
+ name = "Application Insights daily cap reached"
+ resource_group_name = "civil-sdt-aat"
+ scopes = [
+ "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/civil-sdt-aat/providers/Microsoft.Insights/components/civil-sdt-appinsights-aat",
]
+ tags = {
+ "application" = "civil"
+ "autoShutdown" = "true"
+ "builtFrom" = "https://github.com/HMCTS/civil-sdt.git"
+ "businessArea" = "CFT"
+ "contactSlackChannel" = "#civil_contact"
+ "environment" = "staging"
+ "managedBy" = "Civil"
}
+ action {
+ action_group_id = (known after apply)
+ webhook_properties = {
+ "from" = "terraform"
+ "slackChannelId" = "C020H1XLM9S"
}
}
+ criteria {
+ category = "Administrative"
+ level = "Warning"
+ operation_name = "Microsoft.Insights/Components/DailyCapReached/Action"
+ resource_id = "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/civil-sdt-aat/providers/Microsoft.Insights/components/civil-sdt-appinsights-aat"
}
}
Plan: 2 to add, 1 to change, 0 to destroy. |
paulridout
reviewed
Mar 1, 2024
consumers/src/main/java/uk/gov/moj/sdt/cmc/consumers/CMCConsumerGateway.java
Show resolved
Hide resolved
paulridout
reviewed
Mar 15, 2024
There was a problem hiding this comment.
There's three situations where the response will be incorrect:
1.) Results returned from each ConsumerGateway are within maximum limit, but when combined they exceed it. In this case the response will not contain status code 78 (Results truncated - maximum number allowed reached).
2.) Results are returned from ConsumerGateway, but no results returned from CMCConsumerGateway. Assuming that CMCConsumerGateway would return a FeignException, this would cause the response to contain status code 77 (No defence notifications found for requested period) event though the response contained some results.
3.) Results are returned from ConsumerGateway and too many results returned by CMCConsumerGateway. The response would correctly contain status code 78 (from FeignException), but the number of responses would exceed the maximum.
A possible solution for 1 and 3 would be to send a value in the request to CMCConsumerGateway. The value would be the number of results already returned by ConsumerGateway. CMCConsumerGateway would need to subtract this from the default maximum number and use this as the maximum number when determining whether to truncate the results and return status code 78. Alternatively, the subtraction could be done within the SDT code. The value sent to CMCConsumerGateway would then be the maximum number that can be returned without needing to truncate the results and return status code 78.
A possible solution for 2 would be that if CMCConsumerGateway returns status code 77 then check the response from ConsumerGateway. If the response from ConsumerGateway doesn't have a status code of 77 then don't set it to 77.
Plan Result (aat)
|
paulridout
reviewed
Apr 16, 2024
| submitQueryRequest.setResultCount(2000); | ||
| IErrorLog errorLog = new ErrorLog(); | ||
| errorLog.setErrorCode("78"); | ||
| errorLog.setErrorText("maximum number of defences reached"); |
There was a problem hiding this comment.
The error text for code 78 will be "Results truncated - maximum number allowed reached."
There was a problem hiding this comment.
addressed
paulridout
reviewed
Apr 17, 2024
| @@ -433,23 +433,21 @@ private void sendRequestToTargetApp(final ISubmitQueryRequest submitQueryRequest | |||
| SubmitQueryResponse mcolSubmitQueryResponse = | |||
| requestConsumer.submitQuery(submitQueryRequest, connectionTimeOut, requestTimeOut); | |||
|
|
|||
| if (Boolean.TRUE.equals(submitQueryRequest.getBulkCustomer().getReadyForAlternateService())) { | |||
There was a problem hiding this comment.
Should this have been removed? Do we always want to query CMC regardless of whether the customer is ready for it or not?
There was a problem hiding this comment.
no, this should be here, this is what discussed during initial requirements
paulridout
reviewed
Apr 17, 2024
Comment on lines
+518
to
+519
|
|
||
|
|
There was a problem hiding this comment.
Remove one of these blank lines.
There was a problem hiding this comment.
removed
paulridout
reviewed
Apr 17, 2024
| updateResponseStatus(submitQueryRequest, submitQueryResponse, response); | ||
| } catch (FeignException e) { | ||
| boolean noResults = NO_DATA == e.status() || NOT_FOUND == e.status(); | ||
| if (!noResults) { |
There was a problem hiding this comment.
Could not noResults be replaced with a positively named variable instead? Something like resultsExist. It would make the logic a bit easier to understand.
There was a problem hiding this comment.
NO_DATA == e.status() || NOT_FOUND == e.status();
these status codes mean, no data
There was a problem hiding this comment.
It was more the use of a double negative. Double negatives are a bit harder to understand, so I wondered if there was a way of changing it to a positive. For example, "if (resultsExist)" is easier to understand than "if (not noResults)" despite them meaning the same thing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…urned from CMC
Before creating a pull request make sure that:
Please remove this line and everything above and fill the following sections:
JIRA link (if applicable)
Change description
Does this PR introduce a breaking change? (check one with "x")