-
Notifications
You must be signed in to change notification settings - Fork 93
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
feat: add support to log commit stats #205
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stamping for the samples noxfile update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logging bits look good to me, but I haven't worked much with loggers. 🙈
@daniel-sanche would you be able to review this? Are there any best practices to ensure client library logs work nicely with the Observability products?
I think that really depends on where this code is run, since different GCP environments have different logging policies. Many GCP environments work best if the log is printed as structured JSON, but that isn't supported everywhere. If we want to make sure the logs look good in Cloud Logging no matter where it's run, right now the best option is probably to include the logging client library here. But if we want to avoid the new dependency, I'd say the best option would be to print a simple message to stdout, without extra formatting. |
@daniel-sanche Thank you for the feedback! I've removed the formatter and passed the
Users can use a custom logger with a formatter to suit their logging needs, or a custom logger which overloads
I think this is a much better design. WDYT? |
For the most part that seems good to me, I think it's a good idea to just write the stats to stdout, and let the user override formatting/handlers if they decide to. I'm not sure if I understand the |
google/cloud/spanner_v1/session.py
Outdated
txn.commit_stats.mutation_count | ||
) | ||
) | ||
self._database.logger.info(txn.commit_stats) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wnet over this in my other comment, but I'll repeat it here too: The docs for Python Logging say they expect the main msg
argument to be a string. If you want to add a custom object, see the extra
argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's exactly what I want. I've updated the call to use a formatted message and pass the CommitStats
via the extra argument. Thank you for your patience while I learn how to use loggers correctly 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! Yeah this now LGTM from a logging perspective
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
This PR fixes the assertion to use `metadata.backup_info.version_time` instead of `metadata.backup_info.create_time`. It looks it was passing before the backend correctly supported it and I forgot to re-run the tests before merging #148 (whoops!) and so it is currently failing and preventing #205 from being merged: https://source.cloud.google.com/results/invocations/8f0f5dab-1b35-4ce3-bb72-0ce9e79ab89d/targets/cloud-devrel%2Fclient-libraries%2Fpython%2Fgoogleapis%2Fpython-spanner%2Fpresubmit%2Fpresubmit/log
This PR adds support for logging commit statistics when requested by the user via
log_commit_stats
.Commit statistics will be logged at the INFO log level.
The default logger will log commit stats using
sys.stderr
, however the user can pass their own logger when constructing the database if needed.A sample of how to use the feature is given below:
Once
log_commit_stats
is set, commit stats will be logged via the database logger any time commit is called by the library.