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

[PLA-2013] Add config and LRU cache #41

Merged
merged 3 commits into from
Sep 24, 2024
Merged

[PLA-2013] Add config and LRU cache #41

merged 3 commits into from
Sep 24, 2024

Conversation

leonardocustodio
Copy link
Contributor

@leonardocustodio leonardocustodio commented Sep 24, 2024

PR Type

Enhancement, Configuration changes


Description

  • Implemented LRU caching for metadata in chain_info.dart to optimize memory usage.
  • Added logging capabilities across the application to track metadata instantiation and request handling.
  • Introduced environment variable support for server configuration, including number of isolates and bind port.
  • Updated dependencies to include lru_memory_cache, dotenv, and logging.
  • Provided an example .env file for configuration guidance.

Changes walkthrough 📝

Relevant files
Enhancement
chain_info.dart
Implement LRU caching and logging for chain metadata         

lib/decoder/chain_info.dart

  • Introduced LRU caching for metadata to optimize memory usage.
  • Added environment variable support for configuration.
  • Implemented logging for metadata instantiation.
  • +42/-38 
    general.dart
    Add logging for request handling and errors                           

    lib/handler/general.dart

  • Added logging for request handling and error scenarios.
  • Configured logger to capture all log levels.
  • +14/-0   
    Configuration changes
    server.dart
    Load server configuration from environment variables         

    bin/server.dart

  • Added configuration loading from environment variables.
  • Implemented logging for server startup parameters.
  • +34/-3   
    .env.example
    Provide example environment configuration file                     

    .env.example

    • Added example environment variables for configuration.
    +4/-0     
    Dependencies
    pubspec.yaml
    Update dependencies and project version                                   

    pubspec.yaml

  • Updated project version to 2.1.0.
  • Added new dependencies: lru_memory_cache, dotenv, and logging.
  • +8/-2     
    Additional files (token-limit)
    v1032.dart
    ...                                                                                                           

    lib/consts/enjin/production/v1032.dart

    ...

    +3/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

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

    Error Handling
    The implementation of getDecodedMetadata and getChainInfo lacks error handling for potential exceptions from MetadataDecoder.instance.decode. Consider adding try-catch blocks or other error handling mechanisms to manage exceptions and provide fallbacks or error messages.

    Logging Configuration
    The logger is configured globally in multiple files which might lead to redundant configurations and potential conflicts. Consider centralizing the logger configuration to ensure consistency across different parts of the application.

    Configuration Duplication
    There is a duplication in the configuration loading logic in bin/server.dart and lib/decoder/chain_info.dart. It's better to have a single point of configuration loading to avoid inconsistencies and make the code easier to maintain.

    Copy link

    github-actions bot commented Sep 24, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add checks to ensure metadata fields exist before accessing them

    To ensure that the cache key generation is robust and less error-prone, consider
    adding null checks or assertions to ensure that the necessary fields exist in the
    metadata before accessing them. This change prevents potential runtime errors due to
    missing fields.

    lib/decoder/chain_info.dart [18-19]

    -generateKey: (k) =>
    -    "${k.constants['System']!['Version']!.value['spec_name']}-${k.constants['System']!['Version']!.value['spec_version']}",
    +generateKey: (k) {
    +  assert(k.constants['System'] != null && k.constants['System']!['Version'] != null, 'Required metadata fields are missing');
    +  return "${k.constants['System']!['Version']!.value['spec_name']}-${k.constants['System']!['Version']!.value['spec_version']}";
    +},
     
    Suggestion importance[1-10]: 9

    Why: Adding assertions to check for the existence of necessary fields before accessing them is crucial for preventing runtime errors, making this a high-impact suggestion for improving code robustness.

    9
    Maintainability
    Use constants for default values to improve maintainability

    To enhance code maintainability and avoid magic numbers, consider defining constants
    for default values used in parsing environment variables. This approach not only
    makes the code cleaner but also centralizes the configuration values, making them
    easier to manage.

    bin/server.dart [17-20]

    +const defaultNumberOfIsolates = 8;
    +const defaultBindPortNumber = 8090;
     numberOfIsolates =
    -    int.tryParse(env.getOrElse('NUMBER_OF_ISOLATES', () => '8')) ?? 8;
    +    int.tryParse(env.getOrElse('NUMBER_OF_ISOLATES', () => defaultNumberOfIsolates.toString())) ?? defaultNumberOfIsolates;
     defaultBindPort =
    -    int.tryParse(env.getOrElse('DEFAULT_BIND_PORT', () => '8090')) ?? 8090;
    +    int.tryParse(env.getOrElse('DEFAULT_BIND_PORT', () => defaultBindPortNumber.toString())) ?? defaultBindPortNumber;
     
    Suggestion importance[1-10]: 8

    Why: Using constants for default values improves maintainability by centralizing configuration values, making the code cleaner and easier to manage. This is a good practice for reducing potential errors.

    8
    Performance
    Cache decoded metadata to reduce computational overhead

    To improve the efficiency of the getChainInfo function, consider caching the result
    of getDecodedMetadata directly, rather than decoding it each time a cache miss
    occurs. This modification can significantly reduce the computational overhead,
    especially for frequently accessed data.

    lib/decoder/chain_info.dart [50-55]

     if (chainInfo == null) {
       logger.info("Metadata $network v$specVersion instantiated");
    -  final metadata = getDecodedMetadata(network, specVersion);
    +  final metadata = cache.getOrAdd(getSpecName(network, specVersion), () => MetadataDecoder.instance.decode(getDecodedMetadata(network, specVersion)));
       chainInfo = ChainInfo.fromMetadata(metadata);
     }
     
    Suggestion importance[1-10]: 8

    Why: Caching the decoded metadata directly can significantly reduce computational overhead, enhancing performance for frequently accessed data. This is a valuable optimization for improving efficiency.

    8
    Best practice
    Improve variable naming for clarity and explicitness

    Consider using a more descriptive variable name for specPerIsolate and
    specExpireDuration to indicate their units. For example, specPerIsolate could be
    renamed to specsPerIsolate to clarify that it represents the number of specs per
    isolate, and specExpireDuration could be renamed to specExpireDurationSeconds to
    explicitly state the unit of time.

    bin/server.dart [21-24]

    -int specPerIsolate =
    +int specsPerIsolate =
         int.tryParse(env.getOrElse('SPEC_PER_ISOLATE', () => '4')) ?? 4;
    -int specExpireDuration =
    +int specExpireDurationSeconds =
         int.tryParse(env.getOrElse('SPEC_EXPIRE_DURATION', () => '300')) ?? 300;
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability by making variable names more descriptive, which helps in understanding the code better. However, it is not a critical change.

    7

    @leonardocustodio leonardocustodio merged commit fc5e802 into master Sep 24, 2024
    1 check passed
    @leonardocustodio leonardocustodio deleted the PLA-2013 branch September 24, 2024 15:39
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants