-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Ktor-powered REST Datadog module #122
Conversation
internal class HttpError(message: String? = null) : Throwable(message) | ||
|
||
internal interface LogSubmission { | ||
suspend fun submitLogs(logs: List<JsonObject>): Result<Unit, Throwable> |
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.
We return a Result
, but it isn't even used.
There is no persistence or retry mechanism, is that planned in a follow up PR?
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.
Batching, error handling and a retry mechanism are all potential upgrades for later. The idea was to get a first version out and see if we need to prioritize these upgrades after we the first implementation going in Proksy
It wasn't a huge lift to add the Result as a return type on the api request, and it felt like best practice? I know we aren't using it yet but I have a feeling its going to be utilized soon (if we end up needing any of the above features)
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.
LGTM
…ve/add_rest_datadog
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.
Left some comments about potential optimizations, although I realize that might involve bringing back AtomicFu for synchronized
. Sowwy.
Let me know if that doesn't seem feasible. Just ideas, I'm not super attached to them.
@twyatt I think I made all the requested changes and it looks quite a bit better. I went ahead and tested it again (still works as expected) |
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.
OMG! Amazing job! Love how it turned out!
Great idea using the immutable collections library, that made things work out great. Looks like it'd be way more efficient now.
💯 🏅
http | ||
.preparePost("api/v2/logs") { | ||
setBody(logs) | ||
}.executeForResult() |
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.
ultra nit
}.executeForResult() | |
} | |
.executeForResult() |
Adds a new module,
ktor
, which provides a ktor based implementation of a DatadogLogger
, providing through a ktor client directly through the Datadog Log Submission Api