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

fix: support custom exporters #235

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

pradeepbbl
Copy link
Contributor

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
Copy link

google-cla bot commented Jun 15, 2022

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
Copy link
Contributor Author

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

@pradeepbbl pradeepbbl force-pushed the pmishra/support_custom_exporter branch from 99fff91 to c13cbbd Compare June 21, 2022 10:25
@pradeepbbl
Copy link
Contributor Author

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

@lvaylet
Copy link
Collaborator

lvaylet commented Jun 22, 2022

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
Copy link
Contributor Author

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
Copy link
Collaborator

lvaylet commented Jun 22, 2022

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
Copy link
Contributor Author

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

@pradeepbbl pradeepbbl force-pushed the pmishra/support_custom_exporter branch from 9f7ee83 to c13cbbd Compare June 22, 2022 14:15
@lvaylet lvaylet force-pushed the pmishra/support_custom_exporter branch from c13cbbd to 1a350fe Compare July 2, 2022 07:24
@lvaylet
Copy link
Collaborator

lvaylet commented Jul 12, 2022

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
Copy link
Collaborator

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.

currently custom exporter are failing to load due to `capitalize`, this
will add the same logic to `get_exporters` used in `get_backend`.
@lvaylet lvaylet force-pushed the pmishra/support_custom_exporter branch from 1a350fe to fd4ef7c Compare October 19, 2022 09:59
@lvaylet
Copy link
Collaborator

lvaylet commented Oct 19, 2022

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.

@lvaylet lvaylet changed the title fix custom exporter fix: support custom exporters Oct 19, 2022
@lvaylet lvaylet added bug Something isn't working exporter labels Oct 19, 2022
@lvaylet lvaylet merged commit b72b8f4 into google:master Oct 19, 2022
lvaylet pushed a commit that referenced this pull request Nov 25, 2022
Custom exporters are failing to load due to `capitalize`. This PR propagates the `get_backend()` logic to `get_exporters()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exporter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants