Skip to content

Histostat metrics support#3

Merged
smccarthy788 merged 8 commits into
masterfrom
histostat-support
Apr 20, 2022
Merged

Histostat metrics support#3
smccarthy788 merged 8 commits into
masterfrom
histostat-support

Conversation

@smccarthy788

Copy link
Copy Markdown
Contributor

Adds 'histostat' CSV files as a metrics source. An example histostat.csv is included from a NoSqlBench test.

The intended use case is in automation: a Fallout test produces a histostat artifact, processed by Hunter which outputs the change points in json format, which Fallout can then ingest to apply pass/fail logic per test run.

This use case required two separate changes:

  • json output support
  • removing the graphite/grafana config requirement

@guyboltonking

guyboltonking commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

re formatting with black, I note that the github workflow has a completely different linting setup to that done with tox -e lint (which includes running black). Shouldn't the github workflow use tox? Otherwise, we're in a situation where CI applies different linting standards to what you get in a dev environment (a Bad Thing). In general the github workflow should just delegate to tox: is there a good reason why it isn't doing this?

@guyboltonking guyboltonking left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 with a couple of nits. I have a PR that I can submit if you give me access that handles the issues I highlighted earlier with flake8/black/formatting enforcement in general.

Comment thread hunter/config.py Outdated
raise ValueError("graphite.url")
graphite_config = GraphiteConfig(url=config["graphite"]["url"])
if config.get("grafana") is None:
config["gafana"] = {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This misspelling (gafana) was already there, but a) perhaps we could fix it b) the fact that it doesn't seem to break anything suggests it either isn't needed, or it's always been broken and was never used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the last commit. It is broken, but I think we haven't hit it because all of the configs I've seen / used explicitly define a grafana section.

Comment thread hunter/test_config.py Outdated
@guyboltonking guyboltonking mentioned this pull request Apr 20, 2022
…tart of time

The csv samples are dated early 2021, and the default data selection since_time is set to one year prior to runtime. All of the csv samples had 'expired'. We want to keep the default since_time behavior and not have to regularly update the dates in the csv samples.
* remove f strings which don't have placeholders
* fix typo in grafana config
* supress warnings about E203 which is created by black formatting
@smccarthy788 smccarthy788 merged commit a057a23 into master Apr 20, 2022
@smccarthy788 smccarthy788 deleted the histostat-support branch April 20, 2022 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants