Skip to content

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Oct 16, 2018

/cc @salrashid123.

Note that this PR refactors the log entry implementation pretty heavily: log entry classes are now based on named tuples, and should be a) smaller, b) faster, and c) better documented.

Closes #5601 (includes its commits).

Closes #6094.

@tseaver tseaver added the api: logging Issues related to the Cloud Logging API. label Oct 16, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 16, 2018
@salrashid123
Copy link
Contributor

@tseaver thanks
I referened google-cloud-logging in flask-gcp-log-groups , and reused background_thread.py there but i don't think this change will impact it;

@tseaver
Copy link
Contributor Author

tseaver commented Oct 17, 2018

Kokoro did not run the logging unit tests on this last push (see #6231). Merging blocked until that issue is resolved, and the job is re-run.

@tseaver tseaver added do not merge Indicates a pull request not ready for merge, due to either quality or timing. api: bigtable Issues related to the Bigtable API. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Oct 17, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 17, 2018
@tseaver
Copy link
Contributor Author

tseaver commented Oct 17, 2018

Bigtable test flake (#6245) is unrelated to this change.

It is set only when parsing a server response.
Eases comparison of delegated arguments.
Single-entry logger methods pass it.  Batch methods do not, because the
log name for batches is passed as a top-level parameter.
Returns 'None' if the payload is a protobuf message.

'ProtobufEntry.payload_pb' now returns 'None' if the payload is *not*
a protobuf message.

Update system tests accordingly.
- Remove 'entry_type' and 'PAYLOAD_KEY' warts, dispatching instead based
on class to load / save payload.

Normalize inheritance hierarchy, removing spurious named tuples.

Drop 'EmptyEntry':  just use 'LogEntry'.
@tseaver tseaver force-pushed the 6094-logging-additional_entry_fields branch from 11f6ef6 to 30baca5 Compare October 17, 2018 20:51
@tseaver
Copy link
Contributor Author

tseaver commented Oct 17, 2018

Rebased to try getting Kokoro to run only for logging.

@tseaver tseaver removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 17, 2018
@tseaver
Copy link
Contributor Author

tseaver commented Oct 17, 2018

@crwilcox, @chrisrossi This branch is finally right with CI, and should be ready to review once the Kokoro - Logging job completes.

@tseaver tseaver removed the api: bigtable Issues related to the Bigtable API. label Oct 17, 2018
@tseaver tseaver merged commit 06c60ec into master Oct 22, 2018
@tseaver tseaver deleted the 6094-logging-additional_entry_fields branch October 22, 2018 20:40
parthea pushed a commit that referenced this pull request Nov 24, 2025
Use namedtuples to reduce boilerplate in 'logger.Batch',  'logger.Logger', and 'entries' implementations:  remove 'entry_type' and 'PAYLOAD_KEY' warts, dispatching instead based on class to load / save payload.

Parse 'LogEntry.receiveTimestamp' -> 'received_timestamp' attr.  Make 'received_timestamp' default to 'None' for all instances. It is set only when parsing a server response.

Add support for 'LogEntry.traceSampled' field.

Add support for 'source_location' field of log entries.

Add support for 'operation' field of log entries.

Add 'ProtobufEntry.payload_json': Returns 'None' if the payload is a protobuf message.    'ProtobufEntry.payload_pb' now returns 'None' if the payload is *not* a protobuf message.

Drop 'EmptyEntry':  just use 'LogEntry'.

Closes #5601.
Closes #6094.
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. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants