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

tweak: warn if RPC storage connection is unidentified #1591

Merged

Conversation

marcospb19-cw
Copy link
Contributor

@marcospb19-cw marcospb19-cw commented Aug 2, 2024

PR Type

enhancement


Description

  • Added a warning log in PostgresExternalRpcStorage::new to notify if the RPC storage connection URL does not contain the ?app=NAME query parameter.
  • Imported the not utility function to facilitate the new warning condition.

Changes walkthrough 📝

Relevant files
Enhancement
postgres_external_rpc.rs
Add warning for unidentified RPC storage connection URL   

src/eth/storage/postgres_external_rpc/postgres_external_rpc.rs

  • Added a warning log if the RPC storage connection URL does not contain
    a specific query parameter.
  • Imported a new utility function not.
  • +5/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @marcospb19-cw marcospb19-cw marked this pull request as ready for review August 2, 2024 02:10
    @marcospb19-cw marcospb19-cw enabled auto-merge (squash) August 2, 2024 02:10
    Copy link

    github-actions bot commented Aug 2, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit f0fd4ab)

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Logging Enhancement
    The warning log added in PostgresExternalRpcStorage::new should include the reason for the warning in a structured field, such as reason. This will make it easier to filter and analyze logs.

    Initialization Logging
    The initialization of PostgresExternalRpcStorage should log all relevant configurations, not just the URL. This includes connection settings and other critical parameters.

    Copy link

    github-actions bot commented Aug 2, 2024

    Persistent review updated to latest commit f0fd4ab

    Copy link

    github-actions bot commented Aug 2, 2024

    PR Code Suggestions ✨

    Latest suggestions up to f0fd4ab

    CategorySuggestion                                                                                                                                    Score
    Readability
    Replace the not function with the ! operator for negation to improve readability

    The not function usage is unconventional and might be confusing. Consider using the
    ! operator for negation to improve readability.

    src/eth/storage/postgres_external_rpc/postgres_external_rpc.rs [42-44]

    -if not(config.url.contains('?')) {
    +if !config.url.contains('?') {
         tracing::warn!(url = config.url, "url isn't identified with '?app=NAME' query parameter");
     }
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies that using the ! operator for negation is more conventional and improves readability in Rust. The existing code using not is unconventional and might confuse readers.

    9

    Previous suggestions

    Suggestions up to commit f0fd4ab
    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Replace the not function with the ! operator for negation

    The not function is not a standard Rust function for negation. Instead, use the !
    operator to negate the condition.

    src/eth/storage/postgres_external_rpc/postgres_external_rpc.rs [42-44]

    -if not(config.url.contains('?')) {
    +if !config.url.contains('?') {
         tracing::warn!(url = config.url, "url isn't identified with '?app=NAME' query parameter");
     }
     
    Suggestion importance[1-10]: 10

    Why: The not function is not a standard Rust function for negation, and using the ! operator is the correct approach. This suggestion addresses a potential bug in the code.

    10

    @marcospb19-cw marcospb19-cw merged commit acb53b2 into main Aug 2, 2024
    35 checks passed
    @marcospb19-cw marcospb19-cw deleted the tweak-warn-if-rpc-storage-connection-is-unidentified branch August 2, 2024 02:16
    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.

    1 participant