-
Notifications
You must be signed in to change notification settings - Fork 246
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
Added HttpLogger as a configuration for FhirEngine. #1570
Conversation
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.
Just a few nits in the comments, but otherwise this looks great! Thanks a lot!
engine/src/main/java/com/google/android/fhir/sync/remote/FhirHttpLogger.kt
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1570 +/- ##
============================================
- Coverage 36.54% 36.36% -0.18%
Complexity 330 330
============================================
Files 147 149 +2
Lines 4923 4947 +24
Branches 877 879 +2
============================================
Hits 1799 1799
- Misses 2886 2910 +24
Partials 238 238
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
a few small comments. but i also would be happy if there's some test coverage at least to check we're setting up the http logging intercepter in okhttp correctly.
engine/src/main/java/com/google/android/fhir/sync/remote/RemoteFhirService.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/FhirEngineProvider.kt
Outdated
Show resolved
Hide resolved
engine/src/test/java/com/google/android/fhir/sync/remote/MoreHttpLoggerKtTest.kt
Outdated
Show resolved
Hide resolved
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.
Just a small comment, but otherwise looks good to me!
engine/src/test/java/com/google/android/fhir/sync/remote/MoreHttpLoggerKtTest.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/sync/remote/MoreHttpLogger.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/sync/remote/FhirHttpLogger.kt
Outdated
Show resolved
Hide resolved
engine/src/test/java/com/google/android/fhir/sync/remote/MoreHttpLoggerKtTest.kt
Outdated
Show resolved
Hide resolved
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.
the new tests look great! thanks for this aditya!
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #1426
Description
Issue:
There is no way for clients to listen / log the Http communication between engine and server.
Solution:
Added
HttpLogger
as part of server configuration of the FhirEngine to be specified by the developer application. TheHttpLogger
takes a lambda that is called with the log message based on the loggingHttpLogger.Level
specified by the application as part ofHttpLogger.Configuration
Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Choose one: Feature
Screenshots (if applicable)
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.