google / slo-generator

SLO Generator computes SLIs, SLOs, Error Budgets and Burn Rates from supported backends, then exports an SLO report to supported targets.
Apache License 2.0
489 stars 78 forks source link

fix: support custom exporters #235

Closed pradeepbbl closed 2 years ago

pradeepbbl commented 2 years ago

currently, custom exporters are failing to load due to capitalize, this will add the same logic (avoid capitalize custom class name) to get_exporters used in get_backend.

ERROR - Exporter not found in shared config.
Traceback (most recent call last):
  File "/Users/pmishra/Data/failover_scheduler/slo/slo-generator/slo_generator/compute.py", line 127, in export
    raise ImportError('Exporter not found in shared config.')
ImportError: Exporter not found in shared config.
google-cla[bot] commented 2 years ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

pradeepbbl commented 2 years ago

lint error was not related to my change it seems like failing on slo_generator.exporters.cloudevent

pradeepbbl commented 2 years ago

@lvaylet could you please review this MR, let me know if you have any concerns.

lvaylet commented 2 years ago

Hi @pradeepbbl, and thanks a lot for contributing to SLO Generator!

Just so I understand more what you are trying to fix here, can you provide us with an example of a custom exporter that fails to load due to capitalization?

Also, regarding the pylint comment you removed, I am struggling to understand how it is related to this issue.

Finally, @ocervell could you take a quick look when you are back from vacation early next week? I do not want to miss anything critical.

pradeepbbl commented 2 years ago

Hi @lvaylet, thanks for the reply :)

I'm trying to use a custom exporter to send metrics data for POC am using the below sample code

#!/usr/bin/env python3

"""
Custom Exporter
"""

import logging
from logging import Logger
from slo_generator.exporters.base import MetricsExporter

class SloExporter(MetricsExporter):
    """Custom exporter for metrics."""

    logger: Logger = logging.getLogger(__name__)

    def export_metric(self, data):
        """Export data to custom destination.
        Args:
            data (dict): Metric data.
        Returns:
            object: Custom destination response.
        """

        self.logger.info(f"[SloExporter.export_metric] exporting slo metrics: {data}")

        return {"status": "ok", "code": 200}
#!/usr/bin/env python3

"""
Custom SLO provider
"""

import logging
from logging import Logger

class SloProvider:
    logger: Logger = logging.getLogger(__name__)

    def __init__(self, client=None, **kwargs) -> None:
        pass

    def test_slo(self, timestamp, window, slo_config) -> tuple:
        """
        return sample data
        """
        self.logger.info(f"[SloProvider.test_slo] getting slo data for epoch: {timestamp}  window: {window}  config: {slo_config}")
        return (10, 1)

while running the slo-generator CLI am getting the below error (which is caused by capitalization Src.exporter.sloExporter.SloExporterExporter)

(venv) pmishra@FVFFN0YEQ05Q sample_slo % slo-generator compute  -f config/sample_slo_config.yaml -c config/setup.yaml -e
INFO - [SloProvider.test_slo] getting slo data for epoch: 1655892530  window: 3600  config: {'apiVersion': 'sre.google.com/v2', 'kind': 'ServiceLevelObjective', 'metadata': {'name': 'test_slo', 'labels': {'service_name': 'demo', 'slo_name': 'test_slo'}}, 'spec': {'description': 'Test custom provider and exporter', 'backend': 'src.provider.slo_provider.SloProvider', 'method': 'test_slo', 'service_level_indicator': {}, 'error_budget_policy': 'default', 'exporters': ['src.exporter.slo_exporter.SloExporter'], 'goal': 0.999}}
INFO - test_slo                         | 1 hour   | SLI: 90.9091 % | SLO: 99.9 % | Gap: -8.99 % | BR: 90.9 / 9.0 | Alert: 1 | Good: 10       | Bad: 1
ERROR - test_slo                         | 1 hour   | Src.exporter.sloExporter.SloExporterExporter "src.exporter.slo_exporter.SloExporter" failed. | ImportError: Exporter not found in shared config.
ERROR - Exporter not found in shared config.
Traceback (most recent call last):
  File "/Users/pmishra/Data/poc/sample_slo/venv/lib/python3.9/site-packages/slo_generator/compute.py", line 127, in export
    raise ImportError('Exporter not found in shared config.')
ImportError: Exporter not found in shared config.
INFO - Run finished successfully in 0.0s.

and regarding the pylint change as mentioned before it was not related to my change, but am trying to fix the CI check currently it's breaking due to the below error

Ref: https://github.com/google/slo-generator/runs/6982515317?check_suite_focus=true

slo_generator/exporters/cloudevent.py:39:0: R0022: Useless option value for 'disable', 'R0201' was moved to an optional 

extension, see https://pylint.pycqa.org/en/latest/whatsnew/2/2.14/summary.html#removed-checkers. (useless-option-value)
lvaylet commented 2 years ago

Thanks a lot @pradeepbbl for these comprehensive details. Is it OK on your side to wait a few extra days for Olivier's return early next week? I just took over after him on the maintenance part and I do not feel comfortable approving this PR on my own. There might be side effects that I am not aware of. In the meantime, feel free to move forward with your development version.

Regarding the pylint warning, let me fix this in a separate PR. I'd rather keep small PRs that only do one thing, in case we ever need to revert any of them "surgically".

pradeepbbl commented 2 years ago

sure, no worries take your time and will push a revert of pylint change

lvaylet commented 2 years ago

Hi @pradeepbbl, just wanted to let you know that we are not overlooking this PR. I removed the pylint warning in a separate PR so all checks pass on this one. I just need an extra pair of eyes from @ocervell before approving.

ocervell commented 2 years ago

Hi @pradeepbbl, sorry for the delay here ... This looks good to me, maybe @lvaylet re-run the unit tests since the PR is old and check that nothing breaks.

lvaylet commented 2 years ago

Thanks @pradeepbbl and @ocervell.

I just rebased this branch with the latest changes from master. All checks have passed.

I also edited the name of the PR so it matches our semantic commit message policy.