Feature/restart count delta #382

sigilioso · 2022-03-21T15:56:42Z

Closes #379

…ial populate calls

CLAassistant · 2022-03-21T15:56:50Z

All committers have signed the CLA.

paologallinaharbur · 2022-03-24T08:38:55Z

src/scrape/scrape_job_test.go

+func TestRestartCountDeltaValues(t *testing.T) {
+	intgr, err := integration.New("test", "test", integration.InMemoryStore())
+	assert.NoError(t, err)
+	intgr.Clear()


why intgr.Clear() is needed?

As it is (or it should be) a brand new integration, I guess it is not needed. I'm digging into it in this and the populate-test we already had. Thanks!

Removed here: bc62e84

paologallinaharbur · 2022-03-24T08:41:42Z

src/scrape/scrape_job_test.go

@@ -200,6 +233,40 @@ func TestPopulateK8s(t *testing.T) {
 	}
 }

+func TestRestartCountDeltaValues(t *testing.T) {
+	intgr, err := integration.New("test", "test", integration.InMemoryStore())


If I am not wrong two subsequent calls to integration.New() cause a panic (😢 )
If not really needed I would try to avoid using the integration in tests

As integrations are stored globally, it would be advisable to have unique names for them. Otherwise, other test using "test", "test" might cause race/concurrency issues. The Test integration helper does this for you :)

https://github.com/newrelic/nri-kubernetes/blob/main/internal/testutil/integration.go#L23

Thanks! I didn't know. I will fix it here too.

Fixed here: bc62e84

paologallinaharbur · 2022-03-24T08:43:52Z

src/scrape/scrape_job_test.go

+	}
+	for k, values := range valuesInTime {
+		if lenValues := len(values); lenValues > 0 {
+			metrics[k] = values[tg.groupCallsCount%lenValues]


[nit] I would add a comment explaining this, maybe also moving the index calculation to a different line

paologallinaharbur · 2022-03-24T08:45:39Z

src/scrape/scrape_job_test.go

@@ -200,6 +233,40 @@ func TestPopulateK8s(t *testing.T) {
 	}
 }

+func TestRestartCountDeltaValues(t *testing.T) {
+	intgr, err := integration.New("test", "test", integration.InMemoryStore())


can we add t.parallel()?

If you use t.Parallel you'll need to wrap this with a mutex to avoid a race in the SDK. It's not particularly good, but the problem is already solved :)
https://github.com/newrelic/nri-kubernetes/blob/main/internal/testutil/integration.go#L18

Added here: bc62e84

paologallinaharbur

I like the approach, I added just some nits.
By the way after adding a new metric we need to update the spec file 😢 (I would really prefer the nrql being able to deal with cumulative counts in a better way)

roobre · 2022-03-24T12:44:58Z

src/scrape/scrape_job_test.go

@@ -144,18 +145,50 @@ var kubeletSpecs = definition.SpecGroups{
 	"container": metric.KubeletSpecs["container"],
 }

-type testGrouper struct{}
+type testGrouper struct {


I'm probably complaining about code that was already here (sorry!), but I'm finding hard to understand what testGrouper is. Is it a helper? A mock? What does it do? I'd love to see a godoc comment explaining it :)

I just extended the mock we had there... Anyway, you are right it was not obvious and now which it does more stuff it may be confusing. I'll improve naming and maybe include some godocs

Yep, I don't think it needs to be exhaustive since it's unexported and for tests. Oneliners such as // testGrouper is an implementation of the Grouper interface that returns mocked metrics that might change in subsequent calls. will be more than enough :)

roobre · 2022-03-24T12:47:20Z

src/scrape/scrape_job_test.go

+	// PodValuesInTime allows to overwrite some metrics value in each Group execution. Ex: {"isReady": {0, 1, 2, 3}}
+	PodValuesInTime map[string][]interface{}
+	// ContainerValuesInTime allows to overwrite some metrics value in each Group execution. Ex: {"restartCount": {0, 1, 2, 3}}
+	ContainerValuesInTime map[string][]interface{}


Is there a reason to have two identical fields for pod and container specifically? I wonder if we could do this differently so if we need to add values over time for another entity, e.g. Volume, we don't need to add another VolumeValuesInTime field.

After this change 50c8a5a it is a bit more abstract.

We could go further and extract this mock and the corresponding specs to a testutils file and give them some parameters to make it easier to reduce fixtures as needed in different tests... I would include that change in a different PR.

roobre · 2022-03-24T12:47:39Z

src/scrape/scrape_job_test.go

+	return groups, nil
+}
+
+func (tg *testGrouper) buildMetrics(


What does buildMetrics do?

I'm improving docs/naming because if you had to ask... it is not obvious enough ;)

Done: 50c8a5a

roobre · 2022-03-24T12:52:04Z

src/scrape/scrape_job_test.go

@@ -200,6 +233,40 @@ func TestPopulateK8s(t *testing.T) {
 	}
 }

+func TestRestartCountDeltaValues(t *testing.T) {
+	intgr, err := integration.New("test", "test", integration.InMemoryStore())


As integrations are stored globally, it would be advisable to have unique names for them. Otherwise, other test using "test", "test" might cause race/concurrency issues. The Test integration helper does this for you :)

https://github.com/newrelic/nri-kubernetes/blob/main/internal/testutil/integration.go#L23

src/scrape/scrape_job_test.go

roobre

Love the new approach, thanks for addressing the comments! ❤️

sigilioso added 3 commits March 21, 2022 10:02

feat: include restartCountDelta metric

c310994

test: extend scrape job tests to allow metrics overwriting in sequent…

32000af

…ial populate calls

test: include scrape-job test to check restartCountDelta metric behavior

bc62e84

sigilioso linked an issue Mar 21, 2022 that may be closed by this pull request

Add restartCountDelta metric #379

Closed

4 tasks

sigilioso requested a review from a team March 24, 2022 08:00

sigilioso marked this pull request as ready for review March 24, 2022 08:01

paologallinaharbur reviewed Mar 24, 2022

View reviewed changes

roobre reviewed Mar 24, 2022

View reviewed changes

sigilioso added 2 commits March 24, 2022 15:48

chore: improve scrape job tests integration creation

f956a02

test: improve grouper mock maintenability

50c8a5a

sigilioso requested review from paologallinaharbur, roobre and a team March 29, 2022 07:17

roobre approved these changes Mar 29, 2022

View reviewed changes

sigilioso merged commit 7f4c3b3 into main Mar 31, 2022

sigilioso deleted the feature/restartCountDelta branch March 31, 2022 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/restart count delta #382

Feature/restart count delta #382

sigilioso commented Mar 21, 2022 •

edited by roobre

Loading

CLAassistant commented Mar 21, 2022 •

edited

Loading

paologallinaharbur Mar 24, 2022

sigilioso Mar 24, 2022

sigilioso Mar 24, 2022

paologallinaharbur Mar 24, 2022

roobre Mar 24, 2022 •

edited

Loading

sigilioso Mar 24, 2022

sigilioso Mar 24, 2022

paologallinaharbur Mar 24, 2022

paologallinaharbur Mar 24, 2022

roobre Mar 24, 2022

sigilioso Mar 24, 2022

paologallinaharbur left a comment •

edited

Loading

roobre Mar 24, 2022

sigilioso Mar 24, 2022 •

edited

Loading

roobre Mar 24, 2022

roobre Mar 24, 2022

sigilioso Mar 25, 2022

roobre Mar 24, 2022

sigilioso Mar 24, 2022

sigilioso Mar 25, 2022

roobre Mar 24, 2022 •

edited

Loading

roobre left a comment

Feature/restart count delta #382

Feature/restart count delta #382

Conversation

sigilioso commented Mar 21, 2022 • edited by roobre Loading

CLAassistant commented Mar 21, 2022 • edited Loading

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

roobre Mar 24, 2022 • edited Loading

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

paologallinaharbur left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigilioso Mar 24, 2022 • edited Loading

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

roobre Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

roobre left a comment

Choose a reason for hiding this comment

sigilioso commented Mar 21, 2022 •

edited by roobre

Loading

CLAassistant commented Mar 21, 2022 •

edited

Loading

roobre Mar 24, 2022 •

edited

Loading

paologallinaharbur left a comment •

edited

Loading

sigilioso Mar 24, 2022 •

edited

Loading

roobre Mar 24, 2022 •

edited

Loading