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

Fiksclient + digisosapiclient: resttemplate til webclient #339

Merged
merged 34 commits into from
Mar 17, 2021

Conversation

martintveter
Copy link
Contributor

@martintveter martintveter commented Mar 15, 2021

Tar i bruk mockwebserver for test.

Sørger for å bruk springs autoconfigurerte webClientBuilder: WebClient.Builder, da den har satt opp default-metrikker osv

@martintveter martintveter marked this pull request as ready for review March 16, 2021 07:58
@martintveter martintveter requested a review from a team as a code owner March 16, 2021 07:58
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 16, 2021 08:23 Inactive
martintveter added a commit that referenced this pull request Mar 16, 2021
Base automatically changed from bump_sosialhjelp-common to master March 16, 2021 09:15
@martintveter martintveter marked this pull request as draft March 16, 2021 09:48
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 16, 2021 11:33 Inactive
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 16, 2021 11:57 Inactive
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 16, 2021 12:18 Inactive
proxiedWebClient
.mutate()
.baseUrl(clientProperties.fiksDigisosEndpointUrl)
.codecs { it.defaultCodecs().maxInMemorySize(16 * 1024 * 1024) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

by default er maxInMemorySize 256Kb - spring-projects/spring-framework#23961
Det førte til at vi ikke fikk hentet alle søknader for testbruker i dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martintveter martintveter marked this pull request as ready for review March 16, 2021 12:43
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 16, 2021 13:00 Inactive
prøver å ta i bruk springs webClientBuilder, som skal ha en del default greier
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 16, 2021 13:32 Inactive
…ngene gir svært lange loggmeldinger (for lange for Kibana)
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 16, 2021 15:31 Inactive
Comment on lines +56 to +67
.onStatus(HttpStatus::is4xxClientError) {
it.createException().map { e ->
log.warn("Fiks - oppdaterDigisosSak feilet - ${e.statusCode} ${e.statusText}", e)
FiksClientException(e.rawStatusCode, e.message, e)
}
}
.onStatus(HttpStatus::is5xxServerError) {
it.createException().map { e ->
log.warn("Fiks - oppdaterDigisosSak feilet - ${e.statusCode} ${e.statusText}", e)
FiksServerException(e.rawStatusCode, e.message, e)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Disse er jo rimelig like, kan de skrives i en?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jepp! Noe som dette hadde blitt mer konsist

.onErrorMap(WebClientResponseException::class.java) { e ->
    log.warn("Fiks - hentDigisosSak feilet - ${messageUtenFnr(e)}", e)
    if (e.statusCode.is4xxClientError) {
        FiksClientException(e.rawStatusCode, e.message?.feilmeldingUtenFnr, e)
    } else {
        FiksServerException(e.rawStatusCode, e.message?.feilmeldingUtenFnr, e)
    }
}

Tenkte å ta en runde med forenkling av feilhåndtering i webclients etter at alle restemplater er erstattet :)

linemos
linemos previously approved these changes Mar 17, 2021
val error = FrontendErrorMessage(FIKS_ERROR, NOE_UVENTET_FEILET)
return ResponseEntity(error, HttpStatus.INTERNAL_SERVER_ERROR)
}

@ExceptionHandler(FiksClientException::class)
fun handleFiksClientError(e: FiksClientException): ResponseEntity<FrontendErrorMessage> {
log.error("Client-feil ved kall til Fiks", e)
log.error("Client-feil ved kall til Fiks")
Copy link
Contributor

Choose a reason for hiding this comment

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

For å se hva som faktisk er galt pleier jeg å sjekke stacktracen. Feks slik:
image
Er det en annen måte å se den på nå?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

den logges jo i fiksClient også.
log.warn("Fiks - hentDigisosSak feilet - ${messageUtenFnr(e)}", e)
Dessuten, stacktracen er så lang at loggmeldingene blir for lange for kibana 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kan heller fjerne error-loggingen i exceptionHandleren, og endre warn til error i client

Copy link
Contributor

Choose a reason for hiding this comment

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

Der ja, nice. Lurer litt. Disse gir mye mer info. Kan vi endre de til error og heller logge det på linje 60 her som warn.
Kanskje vi egentlig bare skal fjerne disse generelle fra linje 60 etc. 🤔
Det kan vi uansett gjøre i en annen PR.

Men hadde vært kjekt, nå som stacktracen forsvinner, at

log.warn("Fiks - hentDigisosSak feilet - ${messageUtenFnr(e)}", e)

blir logget som error (raskere å finne ut hva som er feil) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

💡

siljee
siljee previously approved these changes Mar 17, 2021
Copy link
Contributor

@siljee siljee left a comment

Choose a reason for hiding this comment

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

Dette ser veldig bra ut 🏆
Veldig mye mer leselig også 🧼✨

@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 17, 2021 10:10 Inactive
@martintveter martintveter dismissed stale reviews from siljee and linemos via 0eb6999 March 17, 2021 10:35
@nais-deploy nais-deploy bot temporarily deployed to dev-sbs:teamdigisos March 17, 2021 10:42 Inactive
@martintveter martintveter merged commit 63ccbb5 into master Mar 17, 2021
@martintveter martintveter deleted the fiksclient_endre_fra_resttemplate_til_webclient branch March 17, 2021 12:17
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.

3 participants