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

Pass Athens's logger to the Redis package #1817

Merged
merged 2 commits into from
Jan 30, 2023
Merged

Conversation

mikesep
Copy link
Contributor

@mikesep mikesep commented Jan 26, 2023

What is the problem I am trying to address?

The Redis package used by pkg/stash/with_redis* occasionally logs bits of information, such as when a Redis Sentinel is discovered or changes, but it uses its own logger by default. If Athens is configured to log in JSON but Redis is logging in plain text, this creates problems when parsing the log output.

How is the fix applied?

Make an adapter struct around pkg/log.Logger to implement the interface that Redis v8 expects, then pass the logger to the WithRedis* functions to call redis.SetLogger.

What GitHub issue(s) does this PR fix or close?

Haven't found an Issue specifically for this.

@mikesep mikesep requested a review from a team as a code owner January 26, 2023 15:26
@r-ashish
Copy link
Member

Thanks for the fix!

The Changes LGTM, just wondering whether you've tested this already to make sure it's working as expected?

@mikesep
Copy link
Contributor Author

mikesep commented Jan 27, 2023

Hi Ashish, thanks for reviewing!

just wondering whether you've tested this already to make sure it's working as expected?

Yes, I've been running this change in my group's Athens instance for the last day or so, and Redis traces are definitely showing up as JSON rather than plain text. 😄

@r-ashish r-ashish merged commit f35a540 into gomods:main Jan 30, 2023
@mikesep mikesep deleted the redis-logger branch January 30, 2023 17:31
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.

None yet

2 participants