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.

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

Converting Logging client->list_entries to iterator. #2636

Merged
merged 3 commits into from
Oct 31, 2016
Merged

Converting Logging client->list_entries to iterator. #2636

merged 3 commits into from
Oct 31, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 28, 2016 •
edited
Loading

@waprin @jonparrott I just realized how weird the logging HTTP/JSON API is. For /entries:list, POST is used instead of GET, and the "query params" come in the payload instead of as query params.

Just as #2633 started, I'm sending this out without fixing unit tests to get it in the hands of reviewers. The unit tests may take me a non-trivial time, but I am working on them.

@dhermes dhermes added api: core api: logging Issues related to the Cloud Logging API. labels Oct 28, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 28, 2016
@theacodes
Copy link
Contributor

iirc logging has some very strange behavior with listing entries, I think @waprin is pretty familiar with it.

@waprin
Copy link
Contributor

waprin commented Oct 28, 2016

Yes specifically it uses the page tokens as a progress indicator, not just as a means of breaking results into pages. This means you can get 0 results on first page, but then 10 results on the next page, so you really always want to check the page token. This is especially true for slower queries, which are most of them without an index e.g. logName= .

@dhermes
Copy link
Contributor Author

dhermes commented Oct 28, 2016

@tseaver @daspecster PTAL

@dhermes
Copy link
Contributor Author

dhermes commented Oct 28, 2016

@waprin So what are the implications for using an Iterator? It'll just take a long time consuming empty pages until the results are available?

@waprin
Copy link
Contributor

waprin commented Oct 28, 2016

@dhermes yeah, if anything I'm thinking this is a very good change because with the iterator people are less likely to get no results on the first page and think there are no results.

@theacodes
Copy link
Contributor

@waprin is there a possibility the iterator will never finish?

daspecster
daspecster reviewed Oct 29, 2016
View reviewed changes
core/google/cloud/iterator.py
method='GET', path=self.path,
query_params=self._get_query_params())
params = self._get_query_params()
if self._HTTP_METHOD == 'GET':

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

core/unit_tests/test_iterator.py
'data': {},
})

def test__get_next_page_bad_http_method(self):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

docs/logging-usage.rst
@@ -68,8 +68,7 @@ Fetch entries for the default project.

>>> from google.cloud import logging
>>> client = logging.Client()
>>> entries, token = client.list_entries() # API call
>>> for entry in entries:
>>> for entry in client.list_entries(): # API call(s)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

logging/google/cloud/logging/_helpers.py
"""Detect correct entry type from resource and instantiate.

:type resource: dict
:param resource: one entry resource from API response

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

logging/google/cloud/logging/_helpers.py
elif 'protoPayload' in resource:
return ProtobufEntry.from_api_repr(resource, client, loggers)

raise ValueError('Cannot parse log entry resource')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

logging/unit_tests/test__gax.py
entry_pb = LogEntry(log_name=self.LOG_NAME,
timestamp = datetime.datetime.utcnow().replace(tzinfo=UTC)
timestamp_pb = _datetime_to_pb_timestamp(timestamp)
entry_pb = LogEntry(log_name=self.LOG_PATH,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

tseaver
tseaver reviewed Oct 31, 2016
View reviewed changes
core/google/cloud/iterator.py
return self.client.connection.api_request(
method=self._HTTP_METHOD,
path=self.path,
data=params)

This comment was marked as spam.

Sign in to view

@dhermes dhermes mentioned this pull request Oct 31, 2016
Closed
@waprin
Copy link
Contributor

waprin commented Oct 31, 2016

@jonparrott can't think of a way

@dhermes
Copy link
Contributor Author

dhermes commented Oct 31, 2016

@tseaver LGTY to merge?

@dhermes dhermes merged commit 016415a into googleapis:master Oct 31, 2016
@dhermes dhermes deleted the logging-iterators-1 branch October 31, 2016 17:15
@dhermes
Copy link
Contributor Author

dhermes commented Oct 31, 2016

As discussed with @daspecster, going to make the docstring updates in a follow-up PR

dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Oct 31, 2016
@dhermes
Docstring update follow-up from PR googleapis#2636.
9a3e3b2
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Oct 31, 2016
@dhermes
Docstring update follow-up from PR googleapis#2636.
d23c1f8
dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Oct 31, 2016
@dhermes
Docstring update follow-up from PR googleapis#2636.
45bf564
@dhermes dhermes mentioned this pull request Nov 14, 2016
Merged
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Merge pull request googleapis#2636 from dhermes/logging-iterators-1
b5902a0 
Converting Logging client->list_entries to iterator.
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Docstring update follow-up from PR googleapis#2636.
c149782
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daspecster daspecster daspecster left review comments

@tseaver tseaver tseaver left review comments

@theacodes theacodes theacodes approved these changes

Assignees

@tseaver tseaver

Labels
api: core api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dhermes @theacodes @waprin @daspecster @tseaver @googlebot