Skip to content

Add a timestamp parameter to the logger. #2860

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
tseaver merged 4 commits into from
Dec 15, 2016
Merged

Add a timestamp parameter to the logger. #2860

tseaver merged 4 commits into from
Dec 15, 2016

Conversation

@Fkawala
Copy link

@Fkawala Fkawala commented Dec 13, 2016

The timestamp parameter is useful when one sends batches of log records. In this case, every log record in a batch will have the same timestamp. The timestamp equals the date of insertion in the google cloud logging API.

If one uses the asctime attribute from the LogRecord instances as timestamp value, a log record would have a propre date.

Added a timestamp parameter to logger. This parameter is usefull when…
f52288a 
… one sends batches of log records. In this case, each log record in a batch will have the same timestamp. This timestamp would be the timestamp of insertion in the google cloud logging API.
@googlebot
Copy link

googlebot commented Dec 13, 2016

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. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 13, 2016
@daspecster daspecster added the api: logging Issues related to the Cloud Logging API. label Dec 13, 2016
@Fkawala
Copy link
Author

Fkawala commented Dec 13, 2016

I signed it!

@googlebot
Copy link

googlebot commented Dec 13, 2016

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 Dec 13, 2016
@daspecster
Copy link
Contributor

daspecster commented Dec 13, 2016

@Fkawala, I noticed that timestamp is always str in the docs. Do you think it would actually be a datetime.datetime at some point?

There are helper functions in google.cloud.core._helpers that might make things easier too.

@tseaver
Copy link
Contributor

tseaver commented Dec 13, 2016

@Fkawala, @daspecster The timestamp paramter to google.cloud.logging.entries._BaseEntry is expected to be an instance of datetime.datetime. It would be best if the equivalent parameter for Logger.log_* was also an instance of datetime.datetime.

@dhermes
Copy link
Contributor

dhermes commented Dec 14, 2016

@waprin PTAL

@Fkawala
Copy link
Author

Fkawala commented Dec 14, 2016 •
edited
Loading

@daspecster @tseaver I'll update the PR to use datetime.datime instances instead of str. I have a question though. Where should we convert the datetime.datime to RFC3339 UTC "Zulu" format (docs)? I guess that the most appropriate would be in the _make_entry_resource. Do you confirm ?

Changed timestamp type from 'str' to 'datetime.datetime'. Convert the…
7ac9c0a 
… timestamp to a RFC3339 string representation in the helper function _make_entry_resource.
@daspecster
Copy link
Contributor

daspecster commented Dec 14, 2016

@Fkawala I think you should convert it as close to receiving it as possible.

@tseaver in thinking about this more, should the log_* methods support both str timestamps and datetime.datetime and just properly convert if needed?

Otherwise the example would be something like

from datetime import datetime

logger.log_text("A simple entry", timestamp=datetime.utcnow())

Or something similar to that.

@Fkawala
Copy link
Author

Fkawala commented Dec 14, 2016

@daspecster Can I get closer than I did in 7ac9c0a ?

@daspecster
Copy link
Contributor

daspecster commented Dec 14, 2016

@Fkawala if a datetime object gets passed to the log_* methods then I think it's good. If a str ends up getting accepted in the log_* methods then we may want to convert them from str->datetime right in those methods.

As I mentioned above, this depends on what we want our usage to look like.

@tseaver
Copy link
Contributor

tseaver commented Dec 14, 2016

@Fkawala The _make_entry_resource helper method is the right place to convert from datetime.datetime to a string: its job is to convert "native" Python objects to their JSON equivalents before putting the entry's repr on the wire.

@Fkawala
Copy link
Author

Fkawala commented Dec 14, 2016

@daspecster ok got it, thanks for the explanation. I'll update the PR If the string compatibility is needed for the log_* methods.

Is there something I could do to for the PR to pass the CI tests ?

@daspecster
Copy link
Contributor

daspecster commented Dec 14, 2016

@Fkawala I think the issue with the Python 3 tests has been resolved. I'll restart the build and see if they clear up.

@waprin
Copy link
Contributor

waprin commented Dec 14, 2016

lgtm

tseaver
tseaver suggested changes Dec 14, 2016
View reviewed changes
logging/unit_tests/test_logger.py Outdated
batch.log_struct(STRUCT)
self.assertEqual(batch.entries,
[('struct', STRUCT, None, None, None, None)])
[('struct', STRUCT, None, None, None, None, None)])

This comment was marked as spam.

Sign in to view

logging/unit_tests/test_logger.py Outdated
batch.log_struct(STRUCT, labels=LABELS, insert_id=IID,
severity=SEVERITY, http_request=REQUEST)
severity=SEVERITY, http_request=REQUEST, timestamp=TIMESTAMP)
self.assertEqual(batch.entries,

This comment was marked as spam.

Sign in to view

logging/unit_tests/test_logger.py Outdated
severity=SEVERITY, http_request=REQUEST, timestamp=TIMESTAMP)
self.assertEqual(batch.entries,
[('struct', STRUCT, LABELS, IID, SEVERITY, REQUEST)])
[('struct', STRUCT, LABELS, IID, SEVERITY, REQUEST, TIMESTAMP)])

This comment was marked as spam.

Sign in to view

logging/unit_tests/test_logger.py Outdated
batch = self._make_one(logger, client=client)
batch.log_proto(message, labels=LABELS, insert_id=IID,
severity=SEVERITY, http_request=REQUEST)
severity=SEVERITY, http_request=REQUEST, timestamp=TIMESTAMP)

This comment was marked as spam.

Sign in to view

logging/unit_tests/test_logger.py Outdated
severity=SEVERITY, http_request=REQUEST, timestamp=TIMESTAMP)
self.assertEqual(batch.entries,
[('proto', message, LABELS, IID, SEVERITY, REQUEST)])
[('proto', message, LABELS, IID, SEVERITY, REQUEST, TIMESTAMP)])

This comment was marked as spam.

Sign in to view

@daspecster
Copy link
Contributor

daspecster commented Dec 14, 2016

@Fkawala now we're cooking with Ci!

You're missing some test coverage in logger.py.
https://travis-ci.org/GoogleCloudPlatform/google-cloud-python/builds/183920499#L378

And the issues that @tseaver mentioned can be seen here
https://travis-ci.org/GoogleCloudPlatform/google-cloud-python/builds/183920499#L423-L432

You can run the tests, lint and coverage commands as mentioned in the CONTRIBUTING.rst.

$ tox -e py27 -- logging
$ tox -e py35 -- logging
$ tox -e cover -- logging
$ tox -e lint

PEP8 compliance fix. Fix tests: timestamp conversion was not covered;…
9cc41ae 
… timestamps were string instead of :class:`datetime.datetime`.
@Fkawala
Copy link
Author

Fkawala commented Dec 15, 2016

@daspecster Thanks for the explanation!
@tseaver I think that I addressed all your points.

tseaver
tseaver reviewed Dec 15, 2016
View reviewed changes
logging/google/cloud/logging/logger.py Outdated
entries = []
for entry_type, entry, labels, iid, severity, http_req, timestamp in self.entries:
for entry_type, entry, labels, iid, severity, http_req, timestamp in \
self.entries:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver tseaver merged commit 8e258d5 into googleapis:master Dec 15, 2016
@tseaver
Copy link
Contributor

tseaver commented Dec 15, 2016

@Fkawala Thank you for the patch!

@Fkawala
Copy link
Author

Fkawala commented Dec 15, 2016 •
edited
Loading

@tseaver I'm happy to contribute, you do truly great work here, thanks!

@Fkawala
Copy link
Author

Fkawala commented Dec 20, 2016

@tseaver, I don't know about the release patterns for this project, when should I expect a release for google.cloud.logging ?

@dhermes
Copy link
Contributor

dhermes commented Dec 21, 2016

Releasing is pretty easy for us, though we probably won't do it until the new year.

@tseaver
Copy link
Contributor

tseaver commented Dec 21, 2016

@dhermes do we need to release core as well, due to the connection refactoring in #2870 / #2875?

@dhermes
Copy link
Contributor

dhermes commented Dec 21, 2016

@tseaver That's pretty likely. Though I want to break off some more chunks before doing a release (e.g. make Connection.api_request a function rather than an instance method)

@Fkawala
Copy link
Author

Fkawala commented Jan 23, 2017

Hi, any update on the expected release of #2860 ?

@Fkawala Fkawala mentioned this pull request Feb 7, 2017
Merged
@dhermes dhermes mentioned this pull request Feb 24, 2017
Merged
19 tasks
@sactre sactre mentioned this pull request Mar 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tseaver tseaver tseaver approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

7 participants

@Fkawala @googlebot @daspecster @tseaver @dhermes @waprin @sactre