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

feat: add support to log commit stats #205

Merged
merged 9 commits into from Feb 23, 2021
Merged

Conversation

larkee
Copy link
Contributor

@larkee larkee commented Jan 7, 2021

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:

database = instance.database(database_id, logger=custom_logger)
database.log_commit_stats = True

Once log_commit_stats is set, commit stats will be logged via the database logger any time commit is called by the library.

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label Jan 7, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 7, 2021
@larkee larkee changed the title Commit stats feat: add support to log commit stats Jan 7, 2021
@larkee larkee requested a review from skuruppu January 7, 2021 07:12
google/cloud/spanner_v1/batch.py Show resolved Hide resolved
google/cloud/spanner_v1/transaction.py Show resolved Hide resolved
google/cloud/spanner_v1/types/spanner.py Outdated Show resolved Hide resolved
@larkee larkee added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 2, 2021
@larkee larkee marked this pull request as ready for review February 2, 2021 12:12
@larkee larkee requested review from a team as code owners February 2, 2021 12:12
@larkee larkee requested review from busunkim96 and removed request for a team February 2, 2021 12:12
Copy link
Contributor

@busunkim96 busunkim96 left a 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.

@skuruppu skuruppu removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 4, 2021
@larkee larkee added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 5, 2021
Copy link
Contributor

@busunkim96 busunkim96 left a 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?

@daniel-sanche
Copy link

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.

@larkee
Copy link
Contributor Author

larkee commented Feb 18, 2021

@daniel-sanche Thank you for the feedback!

I've removed the formatter and passed the CommitStats object directly to the info calls. This means the default behavior will print the CommitStats object to std_out, e.g.

mutation_count: 2

Users can use a custom logger with a formatter to suit their logging needs, or a custom logger which overloads info method and has access to the CommitStats object, e.g.

class CustomLogger(logging.Logger):
    def __init__(self):
        self._last_commit_stats = None

    def info(self, commit_stats):
        self._last_commit_stats = commit_stats
        super().info(commit_stats)
        
    def get_last_commit_stats():
        return self._last_commit_stats

I think this is a much better design. WDYT?

@daniel-sanche
Copy link

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 CustomLogger part though. It's not a good idea to re-interpret the input to info like that in a logger subclass. It seems like you might want to pass in commit_stats in the extra argument. That would allow users to reference it in format strings, or access it in custom Handler classes

txn.commit_stats.mutation_count
)
)
self._database.logger.info(txn.commit_stats)

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

Copy link
Contributor Author

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 👍

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

@larkee larkee added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Feb 22, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 22, 2021
@larkee larkee added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 22, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 22, 2021
@gcf-merge-on-green
Copy link
Contributor

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.

@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 22, 2021
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 23, 2021
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
@larkee larkee added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Feb 23, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 23, 2021
@larkee larkee merged commit 434967e into googleapis:master Feb 23, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants