chore(docs): logging conform Elastic Common Schema #2
Conversation
| { | ||
| "@timestamp": "2024-02-06T17:51:57.0807865+01:00", | ||
| "log.level": "Information", | ||
| "message": "HTTP \"POST\" \"/haalcentraal/api/reisdocumenten/reisdocumenten\" responded 200 in 381.5390 ms", |
There was a problem hiding this comment.
ik denk dat dit niet juist is. in Elastiek/Kibana zie ik altijd veld message staan waarin de hele logregel stringified staat. Dit is dus wat Elastic ontvangen heeft voor de json uit elkaar getrokken is naar losse velden.
There was a problem hiding this comment.
Dit is niet conform de Elastic Common Schema specification. Een stringified string is geen human-readable summary of the event
There was a problem hiding this comment.
Voor begrip met betrekking tot de log message, zie de volgende verwijzingen:
| "hostname": "NUC11TNK" | ||
| }, | ||
| "http": { | ||
| "request.body.content": "{\"type\":\"RaadpleegMetReisdocumentnummer\",\"reisdocumentnummer\":[\"NE3663258\"],\"fields\":[\"reisdocumentnummer\",\"houder\"]}", |
There was a problem hiding this comment.
We willen de request body als object (multi-field) opslaan en daarnaast -voor het geval de afnemer geen valide json stuurt- de request body stringified.
Het logging document zegt nu nog dat moet worden uitgezocht of dit mogelijk is voor de API, maar de wens is er nadrukkelijk wel.
ECS staat volgens mij ook toe dat dit een object (multi-field) is.
There was a problem hiding this comment.
De request body als object staat in het metadata veld. In de ECS specificatie staat dat de type van http.request.body.content tekst moet zijn. Zie: https://www.elastic.co/guide/en/ecs/8.11/ecs-http.html#field-http-request-body-content. En als de ECS Serilog text formatter library wordt gebruikt, dan is ook te zien dat alleen een string als waarde kan worden opgegeven
| "houder" | ||
| ], | ||
| "AdditionalProperties": {}, | ||
| "$type": "RaadpleegMetReisdocumentnummer" |
There was a problem hiding this comment.
waar komt AdditionalProperties vandaan? En de $ voor type?
There was a problem hiding this comment.
Van JSON Schema dat is overgeerfd door de OpenAPI specificatie. In de BRP API specificaties hebben we dit niet op false gezet tbv polymorfisme en om in code te kunnen detecteren dat de request body meer/andere velden bevat dan gespecifeerd in de OpenAPI specificatie.
$type wordt door serializers gebruikt om de naam van het type van een geserialiseerd object door te geven
| "request.id": "0HN176FMTA88H:00000001", | ||
| "request.method": "POST", | ||
| "request.mime_type": "application/json", | ||
| "response.body.content": "{\"type\":\"RaadpleegMetReisdocumentnummer\",\"reisdocumenten\":[{\"reisdocumentnummer\":\"NE3663258\",\"houder\":{\"burgerservicenummer\":\"000000152\"}}]}", |
There was a problem hiding this comment.
De functionele wens is dit als object te leveren, niet als string. ECS staat toe dat dit een multi-field is.
Het logging document stelt daarnaast dat dit alleen moet worden opgeslagen in geval van een foutmelding:
Alleen in geval van een foutmelding (status_code >= 400) als json in response.body
There was a problem hiding this comment.
Zoals aangegeven is de wens niet conform ECS specificatie. Verder, wat is het nut om dit veld alleen te vullen bij status_code >= 400? In deze situaties krijgt je altijd problem json velden. Ik zou verwachten dat je juist bij een status code 200 de response body wilt hebben. Dan zou je kunnen valideren of de velden in de response body overeenkomen met de gevraagde velden in de request body
| "request.id": "0HN176FMTA88I:00000001", | ||
| "request.method": "POST", | ||
| "request.mime_type": "application/json", | ||
| "response.body.content": "{\"invalidParams\":[{\"name\":\"reisdocumentnummer\",\"code\":\"required\",\"reason\":\"Parameter is verplicht.\"}],\"type\":\"https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.1\",\"title\":\"Een of meerdere parameters zijn niet correct.\",\"status\":400,\"detail\":\"De foutieve parameter(s) zijn: reisdocumentnummer.\",\"instance\":\"/haalcentraal/api/reisdocumenten/reisdocumenten\",\"code\":\"paramsValidation\"}", |
There was a problem hiding this comment.
Zie eerdere opmerking: De functionele wens is dit als object te leveren, niet als string. Dat maakt filteren en tonen veel makkelijker/beter.
There was a problem hiding this comment.
Het is geen issue om de response body bij een status_code >= 400 ook destructured toe te voegen aan de logregel. Ik zou daar alleen niet dit veld voor gebruiken. Dat is namelijk niet conform de ECS specificatie, waardoor het ook niet meer mogelijk is om de ECS Serilog Text Formatter te gebruiken en moet er custom code worden geschreven om dit te realiseren.
There was a problem hiding this comment.
Ter info: de request.body.content en response.body.content is wat daadwerkelijk is ontvangen/verzonden, een geserialized json string. Wordt de string gedeserialized naar een object, dan kan je een andere representatie krijgen. Zie jouw opmerking mbt de additionalProperties veld. Deze is toegevoegd aan de code gegenereerd uit onze OpenAPI specificatie omdat wij daar additionalProperties niet expliciet op false hebben gezet. Wat je ook niet ziet zijn alle velden die de waarde null hebben. Velden met null waarden kunnen worden uitgesloten in het omzetten naar een JSON object. Velden met leeg object niet.
There was a problem hiding this comment.
Is er een duidelijke doelstelling voor logging om deze uitwerking aan op te hangen?
- hoe snel zijn de API en de proxy in staat om een antwoord te produceren (latency)?
- hoeveel requests per afnemer, en het bepalen van piekmomenten per afnemer
- welke operaties worden hoe vaak gebruikt en door wie?
- vragen van klanten kunnen beantwoorden over een aansluiting of een specifiek request (succesvol of niet succesvol)
- security: wie probeert er vreemde requests te doen?
Hiervoor is het nodig dat de afnemer id als veld in Elastic komt. Dit gegeven komt uit de OAuth token. De token moet dus niet (alleen) stringified worden gelogd, maar ook als object met de losse onderdelen, waarbij dus met name afnemer id relevant is. @MelvLee ik zie dit nu niet in je voorbeeld. Is het de bedoeling dat dit in user komt?
Hiervoor is nodig dat het hele request wordt gelogd. Voor vragen rond authenticatie en autorisatie is de token nodig. Nu zie ik de token alleen terug bij metadata headers, maar die is niet base64 decoded en dus niet eenvoudig leesbaar. Het zou dan erg helpen de token ook base64 decoded te loggen. Response body content wordt in de voorbeelden in dit PR alleen stringified gelogd. Dat is moeilijk leesbaar. Het zou eenvoudiger leesbaar en vindbaar zijn wanneer de response body ook als object wordt gelogd, met name bij fouten (400+ statussen). Hetzelfde geldt voor het request (uiteraard alleen wanneer het request valide json is). Het lijkt er nu op dat dit alleen wordt gedaan in geval van een succesvol request, maar dit is zeker ook relevant voor foutsituaties.
Hiervoor wordt nu de http status gelogd. Ook afnemer id is hiervoor nodig (zie eerste punt in deze comment). Ik denk dat het gebruikte certificaat (wordt via ssl offloading intern als header meegegeven) hiervoor ook relevant is. In het voorstel worden headers alleen als array geleverd. Voor filteren, groeperen en (in tabel) tonen zou het helpen wanneer de ssl-client-cert header als apart veld in de logging opgenomen wordt. Dit laatste is alleen relevant voor de logging van de API Gateway, want die doet de eerste token controle (request komt alleen door naar API wanneer er een valide OAuth token is en claims daarin matchen met gebruikte certificaat) |
afnemer id is geen standaard JWT claim. Ik verwacht daarom niet dat die automatisch in een User veld komt. Het kan wel als extra object (afnemer id en gemeentecode gegroepeerd) worden opgenomen.
kan ook decoded worden toegevoegd
Persoonlijk zie ik zelf niet het nut van in om de response body destructured toe te voegen aan een log regel bij fout statussen, maar het kan zeker.
Ook hier weet ik niet wat het nut is om bij een bad request te destructuren. Gegevens kunnen kwijtraken door het destructuren naar een object (bijv. er wordt een zoek type opgegeven dat niet wordt ondersteund en niet ondersteunde parameter velden).
Je gaat uit van niemand, maar er is geen garantie dat een consumer wordt gehackt.
Ik ken de Elastic query syntax niet en weet dus niet of daarmee in arrays kan worden gequeried. Zo niet, dan kunnen die als apart veld worden toegevoegd. |
|
Uitwerken van het '230425 Logging BRP API v0.8' specifiek voor in .NET geïmplementeerde sub-systemen van de BRP APIs