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.

Sign up for GitHub

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

Jump to bottom

slo-generator v1.0.0 #403

Merged
merged 113 commits into from
Apr 1, 2020
Merged

slo-generator v1.0.0 #403

merged 113 commits into from
Apr 1, 2020

Conversation

ocervell
Copy link
Contributor

@ocervell ocervell commented Feb 10, 2020 •
edited
Loading

  • Add Stackdriver Service Monitoring backend
  • Reformat SLO reporting in a new dataclass
  • Improve overall documentation
  • Add working samples for each backend
  • Reformat **kwargs blocks in each backend to be less generic (see comment)
  • Add unit tests for ElasticSearch
  • Add unit tests for Prometheus
  • Add unit tests for Stackdriver Service Monitoring

Note: to ease the PR review, you can filter only on .py files. Most of the files (40 out of 58) changed in this PR are Markdown (docs), JSON and YAML files (sample configs).

@googlebot googlebot added the cla: yes All committers have signed a CLA label Feb 10, 2020
morgante
morgante reviewed Mar 18, 2020
View reviewed changes
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

A number of comments/issues.

Also this would be much faster to review if weren't XL.

tools/slo-generator/slo_generator/backends/elasticsearch.py
DEFAULT_DATE_FIELD = '@timestamp'


class ElasticsearchBackend:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the MetricBackend being dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The base class does not add anything to the backends or exporters module, except more complexity having to call the parent's __init__ method, so IMHO not inheriting makes it simpler for people to add more backends if they wish to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think it's valuable to still keep a shared/consistent interface but we don't need to block on that.

@ocervell
Copy link
Contributor Author

ocervell commented Mar 21, 2020 •
edited
Loading

@morgante thanks for your thorough review, I've updated the code based on your remarks. Sorry for the PR size, I have added a lot of documentation to this version to simplify ramp up for customers. I will make smaller PRs for the future versions.

If we ignore the Markdown (documentation) and YAML (samples) files, the git diff for Python files would make the PR way smaller:

git diff --stat master | grep tools/slo-generator/ | grep py
 tools/slo-generator/.pylintrc                      |   7 +-
 tools/slo-generator/setup.py                       |   4 +-
 tools/slo-generator/slo_generator/backends/base.py |  25 -
 tools/slo-generator/slo_generator/cli.py           |  76 ++-
 tools/slo-generator/slo_generator/compute.py       | 225 +------
 tools/slo-generator/slo_generator/report.py        | 275 +++++++++
 tools/slo-generator/slo_generator/utils.py         | 113 +++-
 tools/slo-generator/tests/unit/test_cli.py         |  51 +-
 tools/slo-generator/tests/unit/test_compute.py     | 290 ++++-----
 tools/slo-generator/tests/unit/test_stubs.py       | 270 +++++++++

morgante
morgante approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Thank you for your patience and sorry for the lengthy delay.

@morgante morgante merged commit 90c09c5 into GoogleCloudPlatform:master Apr 1, 2020
@ocervell ocervell deleted the slo-generator-v1.0.0 branch April 1, 2020 07:40
ocervell pushed a commit to ocervell/professional-services that referenced this pull request Sep 25, 2020
@morgante
chore: release slo-generator v1.0.0 (GoogleCloudPlatform#403)
a0abb1a 
* Add support for Service Monitoring API

* Update package for Stackdriver Service Monitoring API support

* Update documentation, fix lints

* Switch support for Stackdriver Service Monitoring and use Python SDK

* Increment version to 1.0.0

* Improve documentation

* Rename secondary error budget policy

* Update linting

* Fix tests

* Remove prometheus-client from deps

* Move report generation to report.py, remove unneeded tests

* Fix sample env var

* Reformat SLO report computations in a dataclass

* Update SLO report dataclass

* Update all backends to have an easier interface

* Fix bug

* Fix elasticsearch broken backend

* Run YAPF on code

* Update sample YAML names

* Improve documentation for SD / SD Service Monitoring and ElasticSearch

* Support basic SLIs in SSM

* Support basic SLIs

* Update docs

* Add samples/ documentation

* Add new samples for log-based metrics

* Run YAPF

* Fix linting

* Fix pylint / flake8 errors

* Fix compute_slo_report

* Update tests

* Fix Prometheus backend

* Add utility to list all SLO files in folder

* Fix case where SLI=0

* Add log-based metric example

* Update Python required version to 3.4

* Add Prometheus exporter to tests + rewrite some tests to be more legible

* Add prometheus client to required dependencies

* remove log-based metric sample for now

* Run YAPF on code

* Fix docstring in SSM

* Fix Pylint + run YAPF

* Run YAPF

* Fix YAPF issue

* Fix **kwargs for SD and SD Service Monitoring

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Improve SLO report __str__ representation

* Update SSM to pick app_engine, mesh_istio sections under measurement

* Improve detection of missing services

* Improve detection of missing services

* Improve detection of missing services

* Improve detection of missing services

* Improve detection of missing services

* Run YAPF

* Add clusterIstio docs + sample for Service Monitoring API

* Rename GKE_CLUSTER_LOCATION to GKE_LOCATION

* Improve documentation for Error Budget and Stackdriver Service Monitoring

* Fix docs (indenting)

* Fix docs (indentation)

* Add example for mesh_istio

* Format md title

* Format YAML in docs

* Format YAML in docs

* Remove TODO.md

* Improve explanation for custom backend / exporter

* Improbe README

* Limit .md line < 80 chars

* Modify Elasticsearch backend to default to @timestamp for date filtering

* Run YAPF

* Tidy up Makefile

* Fix SSM samples

* Add README for using samples

* Tidy up code

* Add unit tests for Stackdriver Service Monitoring

* Fix knit in docs

* Fix samples service_name and feature_name for GKE

* Fix weird behaviour of SSM API with service paths

* Improve log output

* Fix typo in README.md

* Fix typo in README.md

* Lint setup.py

* Add timestamp option to SLO generator CLI

* Add timestamp arg to SLOReport class

* Run YAPF

* Add timestamp arg to SLOReport class

* Update tools/slo-generator/Makefile

Co-Authored-By: Morgante Pell <morgante.pell@morgante.net>

* Fix knits from PR review

* Fix deprecation warning to 2.0

* Loop through names instead of kwargs

* Remove class name shortcut

* Add return value to CLI function + add assertions in tests

* Update CLI tests to check return value of cli method

Co-authored-by: Morgante Pell <morgante.pell@morgante.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morgante morgante morgante approved these changes

Assignees
No one assigned
Labels
cla: yes All committers have signed a CLA size/XXL Denotes a PR that changes 1000+ lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ocervell @morgante @googlebot