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 max_latency to BackgroundThreadTransport #4762

Merged
merged 4 commits into from Feb 13, 2018

Conversation

tcwalther
Copy link
Contributor

@tcwalther tcwalther commented Jan 19, 2018

This PR adds a new parameter max_latency to BackgroundThreadTransport in logging. It can be used to enforce batching by asking the transport to wait for new log messages for a specified amount of time. This is useful to avoid hitting the Stackdriver Logging rate limit.

This PR is written on behalf of Spotify, who, as a company, has signed a corporate CLA. The idea for this PR is based on a support case with Google.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jan 19, 2018
@tcwalther
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jan 19, 2018
items.append(queue_.get_nowait())
elapsed = time.time() - start
timeout = max(0, min_wait_time - elapsed)
items.append(queue_.get(timeout=timeout))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@chemelnucfin chemelnucfin added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: logging Issues related to the Cloud Logging API. labels Jan 21, 2018
@tcwalther tcwalther changed the title Add min_wait_time to BackgroundThreadTransport Add max_latency to BackgroundThreadTransport Jan 22, 2018
@tcwalther
Copy link
Contributor Author

When reviewing this PR, I would be particularly interested in your opinion on how safe it is when a Python processes terminates due to an exception - is there a possibility for log messages getting lost?

@chemelnucfin chemelnucfin added this to To Do in Feature Requests Jan 22, 2018
@tcwalther
Copy link
Contributor Author

I've looked at the code a bit further.

There is a method called _Worker._main_thread_terminated, which claims to send unsent logs. However, it does not seem to do anything other than calling print() a couple of times. It seems to me that this is a bug in the current implementation already.

Thoughts?

:type max_latency: float
:param max_latency: The maximum number of seconds to wait for more than one
item from a queue. This number includes the time required to retrieve
the first item.

This comment was marked as spam.

This comment was marked as spam.

@@ -264,6 +272,34 @@ def test__thread_main_batches(self):
self.assertFalse(worker._cloud_logger._batch.commit_called)
self.assertEqual(worker._queue.qsize(), 0)

def test__thread_main_max_latency(self):

This comment was marked as spam.

This comment was marked as spam.

@theacodes
Copy link
Contributor

There is a method called _Worker._main_thread_terminated, which claims to send unsent logs. However, it does not seem to do anything other than calling print() a couple of times. It seems to me that this is a bug in the current implementation already.

The call to self.stop attempts to block until the background thread has sent pending logs.

When reviewing this PR, I would be particularly interested in your opinion on how safe it is when a Python processes terminates due to an exception - is there a possibility for log messages getting lost?

This is always possible, yes, however, an exception on the main thread (or any thread other than this background thread) will not cause this to lose logs. Lost logs can happen due to one of three reasons:

  1. The request to send the logs fails (due to timeout, etc.)
  2. The main thread dies for any reason (graceful exit, exception, etc.) and sending the pending logs takes longer than grace_period seconds.
  3. There is a bug in the background thread that causes it to terminate without sending logs.

@theacodes
Copy link
Contributor

@tcwalther I re-wrote the test to use monotonic time to keep our unit tests deterministic and fast, totally understand not wanting to do it - it's not an obvious way to write the test.

This looks good, I'm merging. Thank you for doing all of this (and enduring my review).

@dpebot will you merge when tests pass?

@dpebot
Copy link
Contributor

dpebot commented Feb 13, 2018

Okay! I'll merge when all statuses are green and all reviewers approve.

@dpebot dpebot added the automerge Merge the pull request once unit tests and other checks pass. label Feb 13, 2018
@dpebot dpebot assigned dpebot and unassigned theacodes and chemelnucfin Feb 13, 2018
@theacodes theacodes mentioned this pull request Feb 13, 2018
@theacodes theacodes merged commit e9b6f9a into googleapis:master Feb 13, 2018
Feature Requests automation moved this from To Do to Done Q1 2018 Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. automerge Merge the pull request once unit tests and other checks pass. cla: yes This human has signed the Contributor License Agreement. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
No open projects
Feature Requests
  
Done Q1 2018
Development

Successfully merging this pull request may close these issues.

None yet

5 participants