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

Print final response #146

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Print final response #146

merged 3 commits into from
Feb 27, 2024

Conversation

Wambere
Copy link
Contributor

@Wambere Wambere commented Feb 20, 2024

IMPORTANT: Where possible all PRs must be linked to a Github issue

Fixes #143

Engineer Checklist

  • I have run ./gradlew spotlessApply to check my code follows the project's style guide
  • I have built and run the efsity jar to verify my change fixes the issue and/or does not break the application

importer/main.py Outdated
@@ -945,6 +948,9 @@ def main(
else:
logging.error("Empty csv file!")

if only_response:
print(final_response.text)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we pass this to logging instead of printing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pld the idea was to have an option that only print/outputs the final response
So technically you would be able to get both logging and final response, or one or the other depending on what options passed
If we pass it to logging, then we will get everything else that generally gets logged
Unless there is a way to limit logging that I am missing?
Or maybe this is not a use case we want?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, but it's been a while, you can choose to send log to the screen based on the type of log message, so like if you send this to logging.info you can configure logging.info to be printed

But agree on use case, I don't understand that. Who requested this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of running the command with both options

$ python3 main.py --csv_file csv/locations/locations_full.csv --resource_type locations --only_response true --log_level info
INFO:root:Start time: 12:00:40
INFO:root:Starting csv import...
INFO:root:Reading csv file
INFO:root:Returning records from csv file
INFO:root:Processing locations
INFO:root:Building request payload
INFO:root:Posting request-----------------
INFO:root:Request type: GET
INFO:root:Url: https://fhir.labs.smartregister.org/fhir/Location/ba787982-b973-4bd5-854e-eacbe161e297
INFO:root:[200]: SUCCESS!
INFO:root:Posting request-----------------
INFO:root:Request type: GET
INFO:root:Url: https://fhir.labs.smartregister.org/fhir/Location/0a04f1c2-de2a-4869-bab2-763d017e5316
INFO:root:[200]: SUCCESS!
INFO:root:Posting request-----------------
INFO:root:Request type: POST
INFO:root:Url: https://fhir.labs.smartregister.org/fhir
INFO:root:Processing complete!
{
  "resourceType": "OperationOutcome",
  "text": {
    "status": "generated",
    "div": "<div xmlns=\"http://www.w3.org/1999/xhtml\"><h1>Operation Outcome</h1><table border=\"0\"><tr><td style=\"font-weight: bold;\">ERROR</td><td>[]</td><td><pre>HAPI-0550: HAPI-0550: HAPI-0989: Trying to update Location/6c821162-0436-5830-9b4e-ea49027e0a7d/_history/1 but this is not the current version</pre></td>\n\t\t\t</tr>\n\t\t</table>\n\t</div>"
  },
  "issue": [ {
    "severity": "error",
    "code": "processing",
    "diagnostics": "HAPI-0550: HAPI-0550: HAPI-0989: Trying to update Location/6c821162-0436-5830-9b4e-ea49027e0a7d/_history/1 but this is not the current version"
  } ]
}
INFO:root:End time: 12:00:47
INFO:root:Total time: 6.797744 seconds

So if we are to log.info the output instead of printing it, we'd just get the json response below all the other actual logging info. Let me look into filters and try to update the code

Also, side note, print is actually the recommended option to display console output according to the python docs here

This issue came from a discussion I had with @peterMuriuki, maybe he can add more context around use case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we embed this in fhir-web, we'll need its stdout to be in a form that can be easily parsed programmatically, like stringified json. There is a proposal here on what that could look like.

Copy link
Contributor Author

@Wambere Wambere Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterMuriuki can you clarify if there is a use-case where we need only the output, or will we always need the log as well? so we can log the output together with everything else and then later on filter and get it from that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah the use-case only needs the final output, I expect the final output to be a "a summary of the log" so the log is not needed.

@pld
Copy link
Member

pld commented Feb 22, 2024 via email

@Wambere
Copy link
Contributor Author

Wambere commented Feb 23, 2024

Added a logging filter, so now to get just response:

$ python3 main.py --csv_file csv/locations/locations_full.csv --resource_type locations --only_response true
{ "final-response": {
  "resourceType": "OperationOutcome",
  "text": {
    "status": "generated",
    "div": "<div xmlns=\"http://www.w3.org/1999/xhtml\"><h1>Operation Outcome</h1><table border=\"0\"><tr><td style=\"font-weight: bold;\">ERROR</td><td>[]</td><td><pre>HAPI-0550: HAPI-0550: HAPI-0989: Trying to update Location/6c821162-0436-5830-9b4e-ea49027e0a7d/_history/1 but this is not the current version</pre></td>\n\t\t\t</tr>\n\t\t</table>\n\t</div>"
  },
  "issue": [ {
    "severity": "error",
    "code": "processing",
    "diagnostics": "HAPI-0550: HAPI-0550: HAPI-0989: Trying to update Location/6c821162-0436-5830-9b4e-ea49027e0a7d/_history/1 but this is not the current version"
  } ]
}}

And to get all the logs:

$ python3 main.py --csv_file csv/locations/locations_full.csv --resource_type locations --log_level info
INFO:root:Start time: 13:55:39
INFO:root:Starting csv import...
INFO:root:Reading csv file
INFO:root:Returning records from csv file
INFO:root:Processing locations
INFO:root:Building request payload
INFO:root:Posting request-----------------
INFO:root:Request type: GET
INFO:root:Url: https://fhir.labs.smartregister.org/fhir/Location/ba787982-b973-4bd5-854e-eacbe161e297
INFO:root:[200]: SUCCESS!
INFO:root:Posting request-----------------
INFO:root:Request type: GET
INFO:root:Url: https://fhir.labs.smartregister.org/fhir/Location/0a04f1c2-de2a-4869-bab2-763d017e5316
INFO:root:[200]: SUCCESS!
INFO:root:Posting request-----------------
INFO:root:Request type: POST
INFO:root:Url: https://fhir.labs.smartregister.org/fhir
INFO:root:Processing complete!
INFO:root:{ "final-response": {
  "resourceType": "OperationOutcome",
  "text": {
    "status": "generated",
    "div": "<div xmlns=\"http://www.w3.org/1999/xhtml\"><h1>Operation Outcome</h1><table border=\"0\"><tr><td style=\"font-weight: bold;\">ERROR</td><td>[]</td><td><pre>HAPI-0550: HAPI-0550: HAPI-0989: Trying to update Location/6c821162-0436-5830-9b4e-ea49027e0a7d/_history/1 but this is not the current version</pre></td>\n\t\t\t</tr>\n\t\t</table>\n\t</div>"
  },
  "issue": [ {
    "severity": "error",
    "code": "processing",
    "diagnostics": "HAPI-0550: HAPI-0550: HAPI-0989: Trying to update Location/6c821162-0436-5830-9b4e-ea49027e0a7d/_history/1 but this is not the current version"
  } ]
}}
INFO:root:End time: 13:55:44
INFO:root:Total time: 4.120441 seconds

@Wambere
Copy link
Contributor Author

Wambere commented Feb 27, 2024

@pld are there any other changes you would like added to this?

@Wambere Wambere merged commit 174645a into main Feb 27, 2024
4 checks passed
@Wambere Wambere deleted the 143-only-response branch February 27, 2024 17:00
@Wambere Wambere mentioned this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return only response from importer
3 participants