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 label Jan 7, 2021
@google-cla google-cla bot added the cla: yes 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 Jan 7, 2021
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 label Feb 2, 2021
@larkee larkee marked this pull request as ready for review Feb 2, 2021
@larkee larkee requested review from as code owners Feb 2, 2021
@larkee larkee requested review from busunkim96 and removed request for Feb 2, 2021
Copy link
Collaborator

@busunkim96 busunkim96 left a comment

Stamping for the samples noxfile update.

@skuruppu skuruppu removed the do not merge label Feb 4, 2021
@larkee larkee added the do not merge label Feb 5, 2021
Copy link
Collaborator

@busunkim96 busunkim96 left a comment

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?

@busunkim96 busunkim96 requested a review from daniel-sanche Feb 17, 2021
@daniel-sanche
Copy link

@daniel-sanche daniel-sanche commented Feb 17, 2021

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 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?

@larkee larkee requested a review from daniel-sanche Feb 18, 2021
@daniel-sanche
Copy link

@daniel-sanche daniel-sanche commented Feb 18, 2021

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

@larkee larkee Feb 19, 2021

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 kokoro:force-run and removed do not merge labels Feb 22, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Feb 22, 2021
@larkee larkee added the kokoro:force-run label Feb 22, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Feb 22, 2021
@gcf-merge-on-green
Copy link
Contributor

@gcf-merge-on-green gcf-merge-on-green bot commented Feb 22, 2021

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 label Feb 22, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue 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 kokoro:force-run labels Feb 23, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Feb 23, 2021
@larkee larkee merged commit 434967e into googleapis:master Feb 23, 2021
10 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge 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 cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants