-
Notifications
You must be signed in to change notification settings - Fork 48
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
Refactor logs submission #451
Refactor logs submission #451
Conversation
2e62ecf
to
0e9e310
Compare
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.
Thx ! This will be very helpful to improve the log on Jenkins controller side and resolve #344
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.
Dropped some comments
out.flush(); | ||
out.close(); |
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.
Can happen that out.flush()
throws an exception and the OutputStream is never closed?
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.
Seems possible, added a separate try/catch for this
|
||
compressedRequest.write(END_JSON_ARRAY); | ||
compressedRequest.close(); | ||
httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity()); |
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.
httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity()); | |
httpClient.post(logIntakeUrl, headers, "application/json", compressedRequest.toByteArray(), Function.identity()); |
Isn't you need to send the compressedRequest
?
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.
request
is a ByteArrayOutputStream
that contains the actual compressed data.
compressedRequest
is a GZIPOutputStream
that is a wrapper around the request
, responsible for doing the compression.
I think it's the naming that creates the confusion here.
I renamed compressedRequest
to gzip
, but let me know if you can come up with a better name.
if (uncompressedRequestLength + body.length + 2 > PAYLOAD_SIZE_LIMIT) { // + 2 is for comma and array end: ,<payload>] | ||
compressedRequest.write(END_JSON_ARRAY); | ||
compressedRequest.close(); | ||
httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity()); |
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.
httpClient.post(logIntakeUrl, headers, "application/json", request.toByteArray(), Function.identity()); | |
httpClient.post(logIntakeUrl, headers, "application/json", compressedRequest.toByteArray(), Function.identity()); |
Isn't you need to send the compressedRequest?
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.
Same as above
continue; | ||
} | ||
|
||
if (uncompressedRequestLength + body.length + 2 > PAYLOAD_SIZE_LIMIT) { // + 2 is for comma and array end: ,<payload>] |
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.
If we're sending the request compressed, why we're checking the uncompressedReq length?
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.
Because the backend limit is applied to the uncompressed request body, the API docs are explicit about that
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.
Let's add a comment for this
} | ||
|
||
private void fallback(List<String> payloads) { | ||
// cannot establish connection to agent, do nothing |
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.
// cannot establish connection to agent, do nothing | |
// cannot establish connection to API, do nothing |
Maybe? Also, not sure if we should print some errors in this case.
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.
Thanks, the comment was a copy-paste that I overlooked.
The first error will logged in org.datadog.jenkins.plugins.datadog.clients.DatadogApiClient.ApiLogWriteStrategy#handleError
above, which is invoked when the circuit breaker goes from active to inactive state.
In the fallback method we shouldn't be logging it, otherwise we'll spam the logs
continue; | ||
} | ||
|
||
if (uncompressedRequestLength + body.length + 2 > PAYLOAD_SIZE_LIMIT) { // + 2 is for comma and array end: ,<payload>] |
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.
Let's add a comment for this
Requirements for Contributing to this repository
What does this PR do?
Refactors logic that is used for submitting logs to Datadog.
The following improvements are done:
Description of the Change
Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/
label attached. If applicable it should have thebackward-incompatible
label attached.do-not-merge/
label attached.kind/
andseverity/
labels attached at least.