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(common): Log requests and responses #1112

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

dazuma
Copy link
Member

@dazuma dazuma commented Aug 27, 2024

No description provided.

@dazuma dazuma force-pushed the pr/common-logging branch 7 times, most recently from be89659 to 3e27e01 Compare September 9, 2024 23:51
@dazuma dazuma force-pushed the pr/common-logging branch 3 times, most recently from b35abcf to 3a06e57 Compare October 24, 2024 22:46
@dazuma dazuma force-pushed the pr/common-logging branch 2 times, most recently from 9ac59b0 to d1272d2 Compare November 26, 2024 19:52
@dazuma dazuma marked this pull request as ready for review November 26, 2024 19:52
@dazuma dazuma requested a review from a team as a code owner November 26, 2024 19:52
@dazuma dazuma requested a review from aandreassa November 26, 2024 19:52
gapic-common/lib/gapic/logging_concerns.rb Outdated Show resolved Hide resolved

class << self
# @private
def random_uuid4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for SecureRandom.uuid here, and then you don't even need to test it -- the algo looks nice though. We already use in a bunch of places for our repos and its a standard library.

Copy link
Member Author

@dazuma dazuma Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main reason I chose to avoid SecureRandom here is that its internal implementation can theoretically block for brief periods of time (e.g. if /dev/urandom is being used), and for logging purposes we have a fairly strict requirement against that. That said, there is Random::Formatter (i.e. allowing Random.uuid) which I would use except it seems to require Ruby 3.1, and we're not quite scheduled to drop 3.0 support yet until next spring.

gapic-common/lib/gapic/logging_concerns.rb Outdated Show resolved Hide resolved
@@ -64,6 +66,9 @@ class ServiceStub
# be used for intercepting calls before they are executed Interceptors are an EXPERIMENTAL API.
# @param channel_pool_config [::Gapic::ServiceStub:ChannelPool::Configuration] The configuration for channel
# pool. This argument will raise error when `credentials` is provided as a `::GRPC::Core::Channel`.
# @param logger [Logger,:default,nil] An explicit logger to use, or one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action, just saying I probably prefer the docstring order used here.

Having the parameter name after @param is easier to see. Example in google-cloud-ruby

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed. A lot of our early development in google-cloud-ruby used the other ordering (type first). I think it might have been because earlier versions of YARD required that ordering. But I definitely prefer name-first now that YARD supports both.

@dazuma dazuma force-pushed the pr/common-logging branch from 733776a to 7ea86c8 Compare December 5, 2024 20:34
@dazuma dazuma force-pushed the pr/common-logging branch from 7ea86c8 to 68565f9 Compare December 5, 2024 20:57
@dazuma dazuma merged commit fe9e0ec into googleapis:main Dec 5, 2024
10 checks passed
@dazuma dazuma deleted the pr/common-logging branch December 5, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants