Skip to content
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 support for HTTPS compression #190

Merged
merged 1 commit into from
Jan 17, 2020

Conversation

NoisomePossum
Copy link
Contributor

What does this PR do?

Adds support for compression when sending logs over HTTPS.

Motivation

Now that the function sends logs over HTTPS by default we should allow users to compress the logs.

@gaetan-deputier gaetan-deputier requested a review from a team January 14, 2020 15:10
raise Exception("could not scrub the payload")
if DD_USE_COMPRESSION:
try:
data = compress_logs(str(data), DD_COMPRESSION_LEVEL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to convert the data to a string here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll recompress the same batch over and over on retries, we could probably do the compression outside of this method to avoid extra computations on network errors.

Copy link
Contributor Author

@NoisomePossum NoisomePossum Jan 15, 2020

Choose a reason for hiding this comment

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

I went ahead and removed the try block.

For the casting to string, I was getting some type errors for a while from the gzip.compress function when trying to convert data to bytes and casting it to a string first seemed to fix it. I just tried again though without it and everything works ok. Must have been some other error I fixed inadvertently. 🤷‍♂

aws/logs_monitoring/lambda_function.py Outdated Show resolved Hide resolved
aws/logs_monitoring/lambda_function.py Outdated Show resolved Hide resolved
aws/logs_monitoring/lambda_function.py Show resolved Hide resolved
Cleaning up redundant syntax
@NoisomePossum NoisomePossum force-pushed the michael.sha/add-https-compression branch from 5d319d0 to e8a5b92 Compare January 15, 2020 10:30
else:
compression_level = level

return gzip.compress(bytes(batch, 'utf-8'), compression_level)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this method throw an error ? If so, we probably want to catch it here https://github.com/DataDog/datadog-serverless-functions/pull/190/files#diff-618731ddf1c446ef45d09613cb4b189bR339 to avoid retrying indefinitely

Copy link
Contributor

@ajacquemot ajacquemot left a comment

Choose a reason for hiding this comment

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

PR looks nice, thanks for working on this, I think we could probably have moved the encoding logic in DatadogClient to avoid re-encoding the payload for each retry.

@NoisomePossum
Copy link
Contributor Author

I took a stab at putting the compression in DatadogClient but when I do that I get the encoding error I mentioned above : https://a.cl.ly/NQuvpL0k

I think that error gets solved when the compression happens in DatadogHTTPClient because the scrubbing function just before it converts the array of logs into a single string with join. I’m also wondering if the scrubbing would still work if you do the compression in DatadogClient because wouldn't that mean you're trying to scrub text from a string of bytes once you get to that level?

@NoisomePossum NoisomePossum merged commit 7a8368b into master Jan 17, 2020
@NoisomePossum NoisomePossum deleted the michael.sha/add-https-compression branch January 17, 2020 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants