Return 400 instead of 500 on incorrect OTLP payload#6599
Return 400 instead of 500 on incorrect OTLP payload#6599yurishkuro merged 10 commits intojaegertracing:mainfrom
Conversation
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
|
@yurishkuro I think that the issue of the 500 error is coming because of the otlp2traces function called here: I think that if the otlp payload cannot be converted into a JSON, there's definitely an issue with the payload and not the server. That's why I have replaced the status code |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6599 +/- ##
==========================================
- Coverage 96.10% 96.08% -0.03%
==========================================
Files 366 366
Lines 20949 20949
==========================================
- Hits 20133 20128 -5
- Misses 620 624 +4
- Partials 196 197 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
please add a unit test to validate that
|
please make PR title more descriptive |
ADI-ROXX
changed the title
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
|
@yurishkuro I have added a test case in which I am providing a bad otlp payload and then I am checking is the phrase "400 error" is in the response or not. |
cmd/query/app/http_handler_test.go
Outdated
| response := new(any) | ||
| request := "Bad Payload" | ||
| err := postJSON(ts.server.URL+"/api/transform", request, response) | ||
| require.ErrorContains(t, err, "400 error") |
There was a problem hiding this comment.
why do we need to check for string error message instead of the actual status code?
There was a problem hiding this comment.
I have done it in the latest commit titled "Directly check StatusCode". Can you please check it once?
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
|
@yurishkuro In the latest commit, I have directly checked the status code without the need of regex or anything else. Can you review it once? |
cmd/query/app/http_handler_test.go
Outdated
| if err := encoder.Encode(request); err != nil { | ||
| require.Error(t, err) | ||
| } |
There was a problem hiding this comment.
| if err := encoder.Encode(request); err != nil { | |
| require.Error(t, err) | |
| } | |
| require.NoError(t, encoder.Encode(request)) |
There was a problem hiding this comment.
Resolved in the latest commit.
There was a problem hiding this comment.
I expected you'd be able to pattern match on the suggestion and apply it consistently, not just to this line.
Signed-off-by: cs-308-2023 <adityaruhela2003@gmail.com>
yurishkuro
changed the title
Signed-off-by: Yuri Shkuro <github@ysh.us>
2 participants
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test