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

Update Fold + Env to allow for filters for instance status #10

Merged
merged 4 commits into from
Mar 3, 2015

Conversation

oswynb
Copy link

@oswynb oswynb commented Feb 27, 2015

Add a 'Filters' to the Env to supply ceilometer-common with a predicate to filter Instance metrics based on status.

@tranma
Copy link
Contributor

tranma commented Feb 27, 2015

I'm not sure how this is supposed to work. Say you have events:

                                                 query end somewhere here
                                                 v
InstanceActive | InstanceSuspended | InstanceResize 
       ^ 
       query start somewhere here

and InstanceSuspended is not "billable", so you just ignore the time delta between start and InstanceSuspended, but you don't change the accumulator, so you end up adding the delta between start and InstanceResize -- that wouldn't change the result at all, would it?

Can you put an example here so we can see?

@tranma
Copy link
Contributor

tranma commented Feb 27, 2015

Ah, I see. It's a bit convoluted, I can't think of a cleaner way on the top of my head. Regardless, could you add some quickcheck tests, for both instance flavors and non-flavor resources?

@tranma
Copy link
Contributor

tranma commented Feb 27, 2015

if we have quickcheck tests, we can formulate exactly what we're expecting here.

@oswynb
Copy link
Author

oswynb commented Feb 27, 2015

Shall do

@tranma
Copy link
Contributor

tranma commented Feb 27, 2015

tests/SampleData has all the Arbitrary & friends instances

@olorin
Copy link
Contributor

olorin commented Mar 1, 2015

I think this needs a minor bump?

v <- M.lookup k fullResult
v' <- M.lookup k filteredResult
return $ v' <= v) filteredKeys

Copy link
Contributor

Choose a reason for hiding this comment

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

can you put a short comment describing this property -- something to the effect of "filtered results must be subset of ... and all filtered values are less than corresponding values in full results"

then please merge.

@tranma
Copy link
Contributor

tranma commented Mar 3, 2015

sign-off.

$ L.fold foldInstanceFlavor flavorTimedPDs `shouldBe` M.fromList flavorTimedPDsResult
$ L.fold (foldInstanceFlavor $ const True) flavorTimedPDs `shouldBe` M.fromList flavorTimedPDsResult

describe "Filtering instance statuses is sane" $
Copy link
Contributor

Choose a reason for hiding this comment

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

This use of "describe" is odd - normally, describe is used to name the thing which is described by the tests, not assert that the thing is sane.

Copy link
Contributor

Choose a reason for hiding this comment

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

describe "filtering instance statuses" $
prop "is sane" ....

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep.

tranma added a commit that referenced this pull request Mar 3, 2015
Update Fold + Env to allow for filters for instance status
@tranma tranma merged commit 171ec47 into master Mar 3, 2015
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.

None yet

3 participants