Skip to content

Conversation

@bimal12
Copy link

@bimal12 bimal12 commented Feb 7, 2026

Pull harddrive stats from smartctl for HDDs and NVME drives

Copy link
Owner

@miaucl miaucl left a comment

Choose a reason for hiding this comment

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

For a clean PR, please add instructions to the Readme.md.

  • What is the frequency you are planning to run this?
  • Fix the "conflicts"
  • Cleanup unused code as good as possibe (comments are not a problem, but unused code lines are confusing)

Otherwise, I really like this extension and will test it as soon as it is in a clean state and I can run the workflows.

Thanks already @bimal12

main_logger = logging.getLogger("main")
mqtt_logger = logging.getLogger("mqtt")

logging.basicConfig(format="%(asctime)s - %(name)s - %(levelname)s - %(message)s")
Copy link
Owner

Choose a reason for hiding this comment

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

please use the same logging system as already present or create a new logger if you need one

Copy link
Owner

Choose a reason for hiding this comment

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

No need for this line, I use specific purpose loggers and do not want to pollute the default logging config as library.

See following code:

def configure_logger(
    logger: logging.Logger, verbosity: int, logdir: str | None
) -> None:
    """Configure main logger.

    Parameters
    ----------
    logger
        The logger to configure
    verbosity
        The verbosity level
    logdir
        The log directory

    """
    if verbosity >= 5:
        logger.setLevel(logging.DEBUG)
    elif verbosity == 4:
        logger.setLevel(logging.INFO)
    elif verbosity == 3:
        logger.setLevel(logging.WARNING)
    elif verbosity == 2:
        logger.setLevel(logging.ERROR)
    elif verbosity == 1:
        logger.setLevel(logging.CRITICAL)

    # Configure logger
    logger.propagate = False

    log_format = "%(asctime)s - %(name)s - %(levelname)s - %(message)s"
    formatter = logging.Formatter(log_format)

    console_handler = logging.StreamHandler()
    console_handler.setFormatter(formatter)
    logger.addHandler(console_handler)

    if logdir:
        try:
            logdirpath = Path(logdir)
            absolute_logdir = (
                logdirpath.resolve() if not logdirpath.is_absolute() else logdirpath
            )
            absolute_logdir.mkdir(parents=True, exist_ok=True)
            log_file = path.join(absolute_logdir, f"linux2mqtt-{logger.name}.log")
            file_handler = RotatingFileHandler(
                log_file, maxBytes=1_000_000, backupCount=5
            )
            file_handler.setFormatter(formatter)
            logger.addHandler(file_handler)
        except Exception as ex:
            logger.warning(
                "Failed to initialize logging to directory %s : %s",
                logdir,
                str(ex),
            )

Copy link
Author

Choose a reason for hiding this comment

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

I think I removed the line, and pulled in the updated loggers.

self.metric.polled_result = {
**self.harddrive.attributes, # type: ignore[unused-ignore]
}
# self.metric._name = self.harddrive.attributes['model_name']
Copy link
Owner

Choose a reason for hiding this comment

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

unneeded?

def get_score(self):
score = 0

# Critical warnings (bitmask)
Copy link
Owner

Choose a reason for hiding this comment

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

What is your scoring based on? Is it an official baseline?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, just an arbitrary scoring metric that I put together to assist with monitoring drive health.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, in that case it would be helpful to document it somewhere for users to understand :)

Copy link
Author

Choose a reason for hiding this comment

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

I have added this into the readme now.


ata_regex = "^ata.*(?<!part\d)$"
nvme_regex = "^nvme-eui.*(?<!part\d)$"
# potential_disks = os.listdir("/dev/disk/by-id/")
Copy link
Owner

Choose a reason for hiding this comment

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

unneeded?

@github-actions github-actions bot added the 📖 documentation Improvements or additions to documentation label Feb 8, 2026
@bimal12 bimal12 requested a review from miaucl February 8, 2026 03:29
Copy link
Owner

@miaucl miaucl left a comment

Choose a reason for hiding this comment

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

looks better already, still some points open but workflows are already happy :)


self.attributes["Model Name"] = self._attributes["model_name"] # type: ignore
self.attributes["Device"] = self._attributes["device"]["name"] # type: ignore
self.attributes["Model Name"] = self._attributes["model_name"] # type: ignore[index]
Copy link
Owner

Choose a reason for hiding this comment

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

why all this ignore index?

Copy link
Author

Choose a reason for hiding this comment

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

It was what mypy was saying was needed.

error: "type: ignore" comment without error code (consider "type: ignore[index]" instead) [ignore-without-code]

main_logger = logging.getLogger("main")
mqtt_logger = logging.getLogger("mqtt")

logging.basicConfig(format="%(asctime)s - %(name)s - %(levelname)s - %(message)s")
Copy link
Owner

Choose a reason for hiding this comment

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

No need for this line, I use specific purpose loggers and do not want to pollute the default logging config as library.

See following code:

def configure_logger(
    logger: logging.Logger, verbosity: int, logdir: str | None
) -> None:
    """Configure main logger.

    Parameters
    ----------
    logger
        The logger to configure
    verbosity
        The verbosity level
    logdir
        The log directory

    """
    if verbosity >= 5:
        logger.setLevel(logging.DEBUG)
    elif verbosity == 4:
        logger.setLevel(logging.INFO)
    elif verbosity == 3:
        logger.setLevel(logging.WARNING)
    elif verbosity == 2:
        logger.setLevel(logging.ERROR)
    elif verbosity == 1:
        logger.setLevel(logging.CRITICAL)

    # Configure logger
    logger.propagate = False

    log_format = "%(asctime)s - %(name)s - %(levelname)s - %(message)s"
    formatter = logging.Formatter(log_format)

    console_handler = logging.StreamHandler()
    console_handler.setFormatter(formatter)
    logger.addHandler(console_handler)

    if logdir:
        try:
            logdirpath = Path(logdir)
            absolute_logdir = (
                logdirpath.resolve() if not logdirpath.is_absolute() else logdirpath
            )
            absolute_logdir.mkdir(parents=True, exist_ok=True)
            log_file = path.join(absolute_logdir, f"linux2mqtt-{logger.name}.log")
            file_handler = RotatingFileHandler(
                log_file, maxBytes=1_000_000, backupCount=5
            )
            file_handler.setFormatter(formatter)
            logger.addHandler(file_handler)
        except Exception as ex:
            logger.warning(
                "Failed to initialize logging to directory %s : %s",
                logdir,
                str(ex),
            )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📖 documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants