Skip to content

Created a HTTP client to send batch of logs over HTTP to Datadog #87

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 43 commits into from
May 20, 2019
Merged

Created a HTTP client to send batch of logs over HTTP to Datadog #87

merged 43 commits into from
May 20, 2019

Conversation

ajacquemot
Copy link
Contributor

@ajacquemot ajacquemot commented Feb 1, 2019 •
edited
Loading

What does this PR do?

Added the logic to send logs over HTTP to Datadog:

  • Created a HTTP client on top of requests
  • Created a batcher
  • Refactor the TCP client to isolate the retry logic

Motivation

Migrate the integration to th HTTP API

Additional Notes

Need to add unit-tests

@ajacquemot ajacquemot changed the title Created a HTTP client to send batch of logs over HTTP to Datadog [WIP]: Created a HTTP client to send batch of logs over HTTP to Datadog Feb 1, 2019
@ajacquemot ajacquemot requested a review from a team February 1, 2019 17:59
@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch 6 times, most recently from fb5bf9d to 46c62bf Compare February 1, 2019 18:36
ajacquemot
ajacquemot commented Feb 2, 2019
View reviewed changes
@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch from 822eeff to 071065d Compare February 4, 2019 12:13
iksaif
iksaif reviewed Feb 5, 2019
View reviewed changes
Copy link

@iksaif iksaif left a comment

Choose a reason for hiding this comment

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

As discussed offline, I think it would be worth adding a unit test for some of this, even if it has poor coverage and is not (yet) used by a ci.

tmichelet
tmichelet reviewed Feb 5, 2019
View reviewed changes
tmichelet
tmichelet reviewed Feb 5, 2019
View reviewed changes
tmichelet
tmichelet reviewed Feb 5, 2019
View reviewed changes
tmichelet
tmichelet reviewed Feb 5, 2019
View reviewed changes
@tmichelet
Copy link
Contributor

thanks @ajacquemot for this first pass. Let's discuss this when you have some time

@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch 3 times, most recently from a5061c2 to 9e15f59 Compare February 7, 2019 16:34
@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch from 9e15f59 to cff52e8 Compare February 7, 2019 16:39
@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch 2 times, most recently from 2498726 to 9107c87 Compare March 8, 2019 15:36
@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch 2 times, most recently from 759fa06 to 8a8e5dd Compare March 12, 2019 07:33
joyzhang-dd
joyzhang-dd reviewed Mar 25, 2019
View reviewed changes
@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch from 9e236b3 to 825551a Compare April 11, 2019 08:42
joyzhang-dd
joyzhang-dd reviewed Apr 12, 2019
View reviewed changes
aws/logs_monitoring/lambda_function.py Outdated
self.api_key = ddApiKey
self._api_key = api_key
self._scrubber = scrubber
self._timeout = timeout
Copy link

Choose a reason for hiding this comment

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

_timeout is not used anywhere

@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch from afc0069 to ec0aa06 Compare April 12, 2019 13:23
iksaif
iksaif reviewed May 15, 2019
View reviewed changes
aws/logs_monitoring/lambda_function.py Outdated
)
)
except Exception:
pass
Copy link

Choose a reason for hiding this comment

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

log the exception here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

raise an exception instead, we should not forward the logs if we can't build the rule, the user should fix the pattern instead, thoughts ?

@ajacquemot ajacquemot force-pushed the ajacquemot/logs_client_http_switch branch from 9805b12 to 892fe8b Compare May 15, 2019 09:45
gaetan-deputier
gaetan-deputier approved these changes May 15, 2019
View reviewed changes
Copy link
Contributor

@gaetan-deputier gaetan-deputier left a comment

Choose a reason for hiding this comment

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

We might want to add a comment about the retry logic (and how it's controlled by the lambda timeout)

@achntrl
Copy link
Member

achntrl commented May 16, 2019

I find the Exception logic a bit hard to read but I don't have a better solution right now

@ajacquemot ajacquemot merged commit 855ffb4 into master May 20, 2019
@ajacquemot ajacquemot deleted the ajacquemot/logs_client_http_switch branch May 20, 2019 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iksaif iksaif iksaif approved these changes

@tmichelet tmichelet tmichelet left review comments

@joyzhang-dd joyzhang-dd joyzhang-dd left review comments

@achntrl achntrl achntrl approved these changes

@gaetan-deputier gaetan-deputier gaetan-deputier approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ajacquemot @tmichelet @achntrl @iksaif @gaetan-deputier @joyzhang-dd