e2e: Fixed WaitSumMetrics to fail on non existing metric #2256

bwplotka · 2020-03-11T18:54:13Z

Hello Busy Cortexianians 👋

This is kind of tricky as logically sum of non-existing is 0, but also it's super easy to make a
mistake and put the wrong metric and sneakily introduce bug... so I think being strict makes sense here? WDYT?

Alternative is to extend isExpected func(sums ...float64) bool to something like isExpected func(exists bool, sums ...float64) bool
but I think the proposed simplification makes sense here. (:

Also, I think the e2e base unit test should be run all the time not only on integration build,
remove the tag from those in the main e2e core package.

cc @pstibrany @pracucci

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

bwplotka · 2020-03-11T18:55:31Z

Some context thanks to @kakkoyun: thanos-io/thanos#2256

pracucci · 2020-03-12T08:14:22Z

This is kind of tricky as logically sum of non-existing is 0, but also it's super easy to make a
mistake and put the wrong metric and sneakily introduce bug... so I think being strict makes sense here? WDYT?

Thanks @bwplotka! I see the use case but I'm dubious about the fix. As you can see some integration tests fail, and the problem is that some metrics are not immediately exported right after registration. Think about metrics with labels: the first series is exported once a value is tracked for the first time, which may occur after the first time /metrics is called.

bwplotka · 2020-03-12T08:52:01Z

the first series is exported once a value is tracked for the first time, which may occur after the first time /metrics is called.

This is not true...

it's tracked for the first time when you do metrics.WithLabelValues(....) (it will be present as 0) or metrics.WithLabelValues(....).Set(X) (as X).

This means:

You should always initialize labels if you can
Maybe we can assert on metric{expectedlabel=expectedvalue
If you cannot init because the label is so dynamic (I doubt it's really dynamic in e2e tests), maybe you have different metrics that states the state you expect, or we need WaitCountMetric method?

Additionally, think about this:

If you will put the wrong metric your test will pass even though there might be a bug, so assert on 0 without this is really like asserting on nothing. In fact in a case like this when you expect metric to NOT be there, you can even point to totally wrong service and our assertion will work... this does not really make any sense ;p
You should initialize your metrics anyway (it's a good pattern), see: https://www.youtube.com/watch?v=LU6D5cNeHks&t=778s

Anyway, in my free time, I can look on some failing cases to see how I would fix them with my "own tips"... Let me know if that makes sense.

pracucci

I pushed some changes to rework metrics used in integration tests. We have a bunch of global metrics in Cortex which I'm progressively get rid of. Will work on the table manager metrics too, but in a separate PR.

The metrics used in integration tests are the following:

Fixed:

cortex_alertmanager_configs
cortex_dynamo_sync_tables_seconds > introduced cortex_table_manager_sync_success_timestamp_seconds

The label is the tenant which can't be predicted, but used a trick in integration tests to check another related metric first:

cortex_ingester_memory_series_created_total
cortex_ingester_memory_series_removed_total

Already OK:

cortex_ring_tokens_total
cortex_ingester_shipper_uploads_total
cortex_ingester_memory_series
cortex_querier_bucket_store_blocks_loaded
memberlist_client_cluster_members_count
cortex_querier_blocks_index_cache_items
cortex_querier_blocks_index_cache_items_added_total
cortex_querier_blocks_index_cache_hits_total

pracucci · 2020-03-12T08:09:26Z

integration/e2e/scenario_test.go

@@ -1,5 +1,3 @@
-// +build integration


The meaning we gave to this tag was "a test requiring Docker". Does it cause you any trouble keeping it?

Well, my point is that it's not an integration test so technically this tag is wrong.

What if change this tag to requires_docker? (:

Did that (:

I don't care too much how it's called, but feel like this is change just for the change. What if we have integration tests that don't use docker in the future? README.md is not updated.

Could you also update integration/README.md accordingly?

Idea behind using a tag here was to avoid running tests in integration dir when doing go test ./... from main Cortex package. I simply used tag name that matched the directory name.

but feel like this is change just for the change

Just seen this and... what? What do you mean? Why I would put on myself more work to improve something in Cortex for no reason? @pstibrany Sorry, I don't get it.

I just don't feel that renaming the tag was necessary, but ... whatever works. I don't care too much about the name as long as there is a tag in the first place.

gouthamve

LGTM pending on Marco's comment about the integration tag.

bwplotka · 2020-03-12T11:22:46Z

Fixed:

cortex_alertmanager_configs
cortex_dynamo_sync_tables_seconds > introduced cortex_table_manager_sync_success_timestamp_seconds
The label is the tenant which can't be predicted, but used a trick in integration tests to check another related metric first:

cortex_ingester_memory_series_created_total
cortex_ingester_memory_series_removed_total
Already OK:

cortex_ring_tokens_total
cortex_ingester_shipper_uploads_total
cortex_ingester_memory_series
cortex_querier_bucket_store_blocks_loaded
memberlist_client_cluster_members_count
cortex_querier_blocks_index_cache_items
cortex_querier_blocks_index_cache_items_added_total
cortex_querier_blocks_index_cache_hits_total

Thank you @pracucci ! And sorry for more work for you. But I think it actually will help Cortex as well to avoid init metric problem ❤️

cc @kakkoyun as we saved the world again with out talk 💪 😄

kakkoyun · 2020-03-12T11:44:53Z

@bwplotka We're fulfiling our mission is to make the world better place one PR at a time 😁

pracucci · 2020-03-12T11:54:34Z

Makefile

@@ -135,7 +135,7 @@ shell:
 	bash

 configs-integration-test:
-	/bin/bash -c "go test -v -tags 'netgo integration' -timeout 30s ./pkg/configs/... ./pkg/ruler/..."
+	/bin/bash -c "go test -v -tags 'netgo requires_docker' -timeout 30s ./pkg/configs/... ./pkg/ruler/..."


This is a different set of integration tests. You should actually rollback this and replace the -tags=integration in .circleci/config.yml.

pracucci · 2020-03-12T11:55:16Z

integration/e2e/scenario_test.go

@@ -1,5 +1,3 @@
-// +build integration


Could you also update integration/README.md accordingly?

pracucci · 2020-03-12T11:57:32Z

Also, we've just merged another integration test. Could you rebase master and change cortex_dynamo_sync_tables_seconds to cortex_table_manager_sync_success_timestamp_seconds in integration/chunks_storage_backends_test.go, please?

This is kind of tricky as logically sum of non-existing is 0, but also it's super easy to make a mistake and put wrong metric and sneakly introduce bug... so I think being strict makes sense here? WDYT? Alternative is to extend `isExpected func(sums ...float64) bool` to something like `isExpected func(exists bool, sums ...float64) bool` but I think the proposed simplification makes sense here. (: Also I think e2e base unit test should be run all the time not only on integration build, remove the tag from those in main e2e core package. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

Signed-off-by: Marco Pracucci <marco@pracucci.com>

bwplotka · 2020-03-12T12:31:35Z

PTAL (:

bwplotka · 2020-03-12T13:58:06Z

Done, sorry for missed comment.

bwplotka · 2020-03-12T14:13:42Z

--- FAIL: TestChunksStorageAllIndexBackends (12.15s)
    chunks_storage_backends_test.go:58: 
        	Error Trace:	chunks_storage_backends_test.go:58
        	Error:      	Received unexpected error:
        	            	metric cortex_dynamo_sync_tables_seconds not found in table-manager metric page
        	            	github.com/cortexproject/cortex/integration/e2e.(*HTTPService).WaitSumMetrics
        	            		/home/circleci/.go_workspace/src/github.com/cortexproject/cortex/integration/e2e/service.go:549
        	            	github.com/cortexproject/cortex/integration.TestChunksStorageAllIndexBackends
        	            		/home/circleci/.go_workspace/src/github.com/cortexproject/cortex/integration/chunks_storage_backends_test.go:58
        	            	testing.tRunner
        	            		/usr/local/go/src/testing/testing.go:909
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1357
        	Test:       	TestChunksStorageAllIndexBackends
=== RUN   TestGettingStartedSingleProcessConfigWithChunksStorage

Flakiness?

pracucci · 2020-03-12T14:16:55Z

Flakiness?

No, missed comment :)
#2256 (comment)

bwplotka · 2020-03-12T14:17:22Z

damn I did not read the comment properly like 3rd time 🤦‍♂️

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

pracucci

LGTM! Once CI pass we can merge it.

pstibrany · 2020-03-12T14:42:00Z

Thanks Bartek, this is useful.

bwplotka · 2020-03-12T15:14:51Z

Thanks ❤️

…ct/cortex#2256) * e2e: Fixed WaitSumMetrics to fail on non existing metric This is kind of tricky as logically sum of non-existing is 0, but also it's super easy to make a mistake and put wrong metric and sneakly introduce bug... so I think being strict makes sense here? WDYT? Alternative is to extend `isExpected func(sums ...float64) bool` to something like `isExpected func(exists bool, sums ...float64) bool` but I think the proposed simplification makes sense here. (: Also I think e2e base unit test should be run all the time not only on integration build, remove the tag from those in main e2e core package. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Fixed init of the metrics used in integration tests Signed-off-by: Marco Pracucci <marco@pracucci.com> * Moved build tag to `required_docker` to be explicit. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: Marco Pracucci <marco@pracucci.com>

pull-request-size bot added the size/S label Mar 11, 2020

pracucci force-pushed the wait-sum-metric-fix branch from d339d57 to 57b0f7d Compare March 12, 2020 10:57

pracucci reviewed Mar 12, 2020

View reviewed changes

gouthamve approved these changes Mar 12, 2020

View reviewed changes

pracucci reviewed Mar 12, 2020

View reviewed changes

bwplotka force-pushed the wait-sum-metric-fix branch from dc2d6bd to f570616 Compare March 12, 2020 12:00

bwplotka and others added 2 commits March 12, 2020 12:16

Fixed init of the metrics used in integration tests

c22badd

Signed-off-by: Marco Pracucci <marco@pracucci.com>

bwplotka force-pushed the wait-sum-metric-fix branch from f570616 to 55f7276 Compare March 12, 2020 12:16

bwplotka force-pushed the wait-sum-metric-fix branch from 55f7276 to 5e2b851 Compare March 12, 2020 13:58

Moved build tag to required_docker to be explicit.

16a9a44

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>

bwplotka force-pushed the wait-sum-metric-fix branch from 5e2b851 to 16a9a44 Compare March 12, 2020 14:18

pracucci approved these changes Mar 12, 2020

View reviewed changes

gouthamve merged commit 9f4d65b into cortexproject:master Mar 12, 2020

bwplotka deleted the wait-sum-metric-fix branch March 12, 2020 15:14

kakkoyun mentioned this pull request Mar 13, 2020

WIP: e2e tests: Upgrade Cortex e2e test dependency thanos-io/thanos#2264

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: Fixed WaitSumMetrics to fail on non existing metric #2256

e2e: Fixed WaitSumMetrics to fail on non existing metric #2256

bwplotka commented Mar 11, 2020

bwplotka commented Mar 11, 2020

pracucci commented Mar 12, 2020

bwplotka commented Mar 12, 2020

pracucci left a comment

pracucci Mar 12, 2020

bwplotka Mar 12, 2020

bwplotka Mar 12, 2020

pstibrany Mar 12, 2020

pracucci Mar 12, 2020

pstibrany Mar 12, 2020

bwplotka Mar 13, 2020 •

edited

Loading

bwplotka Mar 13, 2020

pstibrany Mar 13, 2020

gouthamve left a comment

bwplotka commented Mar 12, 2020

kakkoyun commented Mar 12, 2020

pracucci Mar 12, 2020

bwplotka Mar 12, 2020

pracucci Mar 12, 2020

pracucci commented Mar 12, 2020

bwplotka commented Mar 12, 2020

bwplotka commented Mar 12, 2020

bwplotka commented Mar 12, 2020

pracucci commented Mar 12, 2020

bwplotka commented Mar 12, 2020

pracucci left a comment

pstibrany commented Mar 12, 2020

bwplotka commented Mar 12, 2020

e2e: Fixed WaitSumMetrics to fail on non existing metric #2256

e2e: Fixed WaitSumMetrics to fail on non existing metric #2256

Conversation

bwplotka commented Mar 11, 2020

bwplotka commented Mar 11, 2020

pracucci commented Mar 12, 2020

bwplotka commented Mar 12, 2020

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouthamve left a comment

Choose a reason for hiding this comment

bwplotka commented Mar 12, 2020

kakkoyun commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci commented Mar 12, 2020

bwplotka commented Mar 12, 2020

bwplotka commented Mar 12, 2020

bwplotka commented Mar 12, 2020

pracucci commented Mar 12, 2020

bwplotka commented Mar 12, 2020

pracucci left a comment

Choose a reason for hiding this comment

pstibrany commented Mar 12, 2020

bwplotka commented Mar 12, 2020

bwplotka Mar 13, 2020 •

edited

Loading