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 math operations on results of aggr #2306

Merged
merged 7 commits into from Sep 20, 2018

Conversation

Projects
None yet
2 participants
@hermanschaaf
Copy link
Contributor

hermanschaaf commented Sep 11, 2018

This PR fixes a bug in aggr that causes math operations on the resulting series to fail. It also adds tests for such cases, and refactors the aggr tests to be table-driven.

Without this fix, math operations cause an error like expr: opentsdb: bad tag: color. The tests included here were failing before the fix with such an error.

CC @kylebrandt

Herman Schaaf added some commits Sep 6, 2018

Herman Schaaf
Fix opentsdb tags in aggr function
Without this fix, math operations cause an error 'expr: opentsdb: bad tag: partner' to be returned
Herman Schaaf
Add test case for no groups
And refactor aggr tests to be table-driven

@hermanschaaf hermanschaaf changed the title Fix aggr tags Fix math operations on results of aggr Sep 11, 2018

Herman Schaaf
@hermanschaaf

This comment has been minimized.

Copy link
Contributor Author

hermanschaaf commented Sep 11, 2018

The tests are failing due to timeouts, doesn't seem to have any real issues.

Herman Schaaf
@hermanschaaf

This comment has been minimized.

Copy link
Contributor Author

hermanschaaf commented Sep 17, 2018

@kylebrandt how does this look to you? Would be nice to get the fix in 🙇

@kylebrandt

This comment has been minimized.

Copy link
Member

kylebrandt commented Sep 17, 2018

Looks like one of the test files just needs a gofmt

@hermanschaaf

This comment has been minimized.

Copy link
Contributor Author

hermanschaaf commented Sep 17, 2018

Ah, was that the issue. Thanks, it's gofmt-ed now.

@hermanschaaf

This comment has been minimized.

Copy link
Contributor Author

hermanschaaf commented Sep 17, 2018

Or not... maybe I need to upgrade to Go1.10 on this machine

Herman Schaaf
@hermanschaaf

This comment has been minimized.

Copy link
Contributor Author

hermanschaaf commented Sep 18, 2018

Alright, it's fixed now 👍 Sorry for the delay

@kylebrandt kylebrandt merged commit 1d6c31a into bosun-monitor:master Sep 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.