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

Add prometheus metrics test into E2E #5673

Merged
merged 19 commits into from
Apr 29, 2020
Merged

Add prometheus metrics test into E2E #5673

merged 19 commits into from
Apr 29, 2020

Conversation

0xKiwi
Copy link
Contributor

@0xKiwi 0xKiwi commented Apr 29, 2020

What type of PR is this?
Improvement

What does this PR do? Why is it needed?
This PR improves E2E testing by adding metric checks, to ensure breaking changes aren't added that cause an increased amount of ssz topic errors, and memory usage.

Which issues(s) does this PR fix?

Fixes #5635

@0xKiwi 0xKiwi requested a review from a team as a code owner April 29, 2020 03:47
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Just 1 suggestion, otherwise looks good

rauljordan
rauljordan previously approved these changes Apr 29, 2020
return -1, fmt.Errorf("did not find requested text %s in %s", topic, pageContent)
}
endOfTopic := startIdx + len(topic)
fmt.Println(pageContent[startIdx:endOfTopic])
Copy link
Member

Choose a reason for hiding this comment

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

is fmt.Println here intended or just for debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, thanks.

{
name: "memory usage",
topic: "go_memstats_alloc_bytes",
value: 100000000, // 100 Mb
Copy link
Member

Choose a reason for hiding this comment

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

100MB only? I guess since it's only a few validators

Copy link
Member

Choose a reason for hiding this comment

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

how do you get 100mb ? is there a better way to derive expected memory usage other than hardcoding it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran it and it seemed to be around 40mb normally. And yeah this is for only 64 validators so thats why its slow low.

@@ -71,7 +71,8 @@ func runEndToEndTest(t *testing.T, config *types.E2EConfig) {
}
// Small offset so evaluators perform in the middle of an epoch.
epochSeconds := params.BeaconConfig().SecondsPerSlot * params.BeaconConfig().SlotsPerEpoch
genesisTime := time.Unix(genesis.GenesisTime.Seconds+int64(epochSeconds/2), 0)
// Adding a half slot here to ensure the requests are in the middle of an epoch.
genesisTime := time.Unix(genesis.GenesisTime.Seconds+int64(epochSeconds/2+(params.BeaconConfig().SecondsPerSlot/2)), 0)
Copy link
Member

Choose a reason for hiding this comment

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

this is genesis time + half_epoch + half_slot . Can you clarify why you are doing this , this way ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added clearer comments.

@@ -16,7 +16,7 @@ func TestEndToEnd_Long_MinimalConfig(t *testing.T) {
testutil.ResetCache()
params.UseMinimalConfig()

epochsToRun := 20
epochsToRun := 100
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't matter right? It gets set from ENV variable

Suggested change
epochsToRun := 100
var epochsToRun int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah my bad was for testing

nisdas
nisdas previously approved these changes Apr 29, 2020
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

lgtm

nisdas
nisdas previously approved these changes Apr 29, 2020
@prylabs-bulldozer prylabs-bulldozer bot merged commit 84e51a5 into master Apr 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the e2e-metrics branch April 29, 2020 13:42
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.

Add metrics testing for e2e
5 participants