chore: introduce in-memory stats for testing #2735

lvrach · 2022-11-24T09:51:10Z

Description

This PR introduces an implementation of stats.Stats and stats.Measurements, where the metrics are kept in a simple in-memory store to assist with testing stats side-effects.

Motivation

The current method of testing stats is to use an auto-generated mock:

	ctrl := gomock.NewController(t)
	mockStats := mock_stats.NewMockStats(ctrl)
	mockMeasurement := mock_stats.NewMockMeasurement(ctrl)

and latter

				mockStats.EXPECT().NewTaggedStat(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return(mockMeasurement)
				mockMeasurement.EXPECT().Count(3).Times(1)

Apart from this approach being a bit complex, the primary issue is the fragility and extensibility of this approach.

Imagine a new stats is introduced, which shouldn't be uncommon. The assertion above will fail and to fix it, you need to do something like:

				mockStats.EXPECT().NewTaggedStat("the_counter", gomock.Any(), gomock.Any()).Times(1).Return(mockMeasurement)

	mockMeasurementSecond := mock_stats.NewMockMeasurement(ctrl)
				mockStats.EXPECT().NewTaggedStat("the_timer", gomock.Any(), gomock.Any()).Times(1).Return(mockMeasurementSecond)
				mockMeasurementSecond.EXPECT().SendTiming(now).Times(1)

You can easily understand how those tests can become unmanageable.

Proposed solution

   	store := &memstats.Store{}
       // inject into stats.Stats
       // run your code
       require.Equal(t, 1, store.Get("stat_name, tags.Tags{"key": "value"}).LastValue)

This approach has the following benefits:

We test exactly the expected metric we will send. name, and tags should match our expectations.
Easy to extend your tests to new measurements.
Ignore metrics we are not interested in testing.

Limitations

This is not meant to run in production; the implementation is not performant.

Notion Ticket

https://www.notion.so/rudderstacks/memory-stats-for-tests-2846f156181f4843a848471660529d61

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

codecov · 2022-11-24T10:15:49Z

Codecov Report

Base: 46.85% // Head: 46.74% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (7f0c52e) compared to base (9f006e5).
Patch coverage: 91.72% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2735      +/-   ##
==========================================
- Coverage   46.85%   46.74%   -0.11%     
==========================================
  Files         298      299       +1     
  Lines       48943    49072     +129     
==========================================
+ Hits        22931    22940       +9     
- Misses      24555    24672     +117     
- Partials     1457     1460       +3

Impacted Files	Coverage Δ
regulation-worker/internal/delete/api/api.go	`67.53% <57.14%> (-13.39%)`	⬇️
services/oauth/oauth.go	`53.88% <76.92%> (-9.11%)`	⬇️
services/stats/memstats/stats.go	`95.04% <95.04%> (ø)`
regulation-worker/cmd/main.go	`68.25% <100.00%> (+0.51%)`	⬆️
processor/stash/stash.go	`44.56% <0.00%> (-22.10%)`	⬇️
config/backend-config/namespace_config.go	`67.00% <0.00%> (-3.00%)`	⬇️
jobsdb/migration.go	`77.28% <0.00%> (-1.31%)`	⬇️
jobsdb/backup.go	`75.45% <0.00%> (-0.37%)`	⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

fracasula

Overall looks good to me and I like that this would simplify our tests. Personally I find gomock to be very unfriendly and I tend to prefer moq a lot more. Not sure if with moq you'd still feel the need to do all this, perhaps it is worth checking before merging this (just in case since it might help with other cases too).

So mostly two things:

please check moq, let me know if it can help somehow
beware of reading LastValue directly in the tests. if some code is still running the write is protected by the library via a mutex but in the test you're reading it without passing through the mutex. it's usually fine in unit tests, less fine in integration tests where the code that you're testing might still be running while you make your assertions. that could cause a failure when running tests of that kind with -race

In any case just reading the last value in some cases feels a bit fragile so perhaps a race failure is OK as it would force us to review the test. Anyhow, this could be a good starting point, imo we can add more on it if needed down the road.

fracasula · 2022-11-24T16:04:51Z

services/stats/memstats/stats.go

+	if m.Type != stats.CountType {
+		panic("operation Increment not supported for measurement type:" + m.Type)
+	}
+


This check is redundant.

Suggested change

if m.Type != stats.CountType {

panic("operation Increment not supported for measurement type:" + m.Type)

}

Why do you say this check is redundant?

Because right after this if you're doing m.Count(1) and the Count method is already checking whether the stat is of the right type. We can remove this if and just call Count, right?

I see; I did it this way cause the panic would be a bit misleading otherwise.

m.Increment()

Will panic with operation Count not supported for measurement type:" + m.Type

fracasula · 2022-11-24T16:09:25Z

services/stats/memstats/stats.go

+	if m.Type != stats.TimerType {
+		panic("operation End not supported for measurement type:" + m.Type)
+	}
+


This check is redundant.

fracasula · 2022-11-24T16:09:36Z

services/stats/memstats/stats.go

+	if m.Type != stats.TimerType {
+		panic("operation Since not supported for measurement type:" + m.Type)
+	}
+


This check is redundant.

please check moq, let me know if it can help somehow

Overall I prefer moq for generating mocks. I still find it will add unnecessary complexity to the alerts. The reason is stats and measurement are very coupled, the problem appears when you have to deal with multiple metrics. Essentially the logic that is implemented here should be repeated one way or the other, in all the test cases.

Ensuring that the correct measurement (name, tags) is changed.

Ignoring stats we are not interested in.

Thought I could have used the moq, to build the measurement implementation.

Given the frequency and complexity of testing stats, I would go ahead with this custom implementation. Maybe we should have the same for logging.

beware of reading LastValue directly in the tests. if some code is still running the write is protected by the library via a mutex but in the test you're reading it without passing through the mutex. it's usually fine in unit tests, less fine in integration tests where the code that you're testing might still be running while you make your assertions. that could cause a failure when running tests of that kind with -race

In any case just reading the last value in some cases feels a bit fragile so perhaps a race failure is OK as it would force us to review the test. Anyhow, this could be a good starting point, imo we can add more on it if needed down the road.

Good point, it slipped my mind, I will make sure it is thread-safe.

fracasula · 2022-11-24T16:12:30Z

services/stats/memstats/stats.go

+		now: ms.Now,
+	}
+
+	ms.byKey[name+tags.String()] = m


Probably a overkilling but I'd generate they key with a function:

func (*Store) getKey(name string, tags stats.Tags) string { return name+tags.String() }

atzoum

It would be nice before merging it to use it in at least one test :)

atzoum · 2022-11-25T07:24:29Z

services/stats/memstats/stats.go

+
+var _ stats.Measurement = (*Measurement)(nil)
+
+type Store struct {


all fields in the Store struct could be unexported and intialization could be taken care once and for all by a producer function using Opts if necessary. This would eliminate the need for the init method being called by other methods and the sync.Once field e.g.

type Opt func(*Store) var WithNow = func(now func() time.Time) Opt { return func(s *Store) { s.now = now } } // New returns a new stats.Stats implementation that stores stats in-memory. func New(opts ...Opt) *Store { s := &Store{ byKey: make(map[string]*measurement), now: time.Now, } for _, opt := range opts { opt(s) } return s } type Store struct { mu sync.Mutex byKey map[string]*measurement now func() time.Time }

and other packages could simply do

store := memstats.New( memstats.WithNow(func() time.Time { return now }))

Your suggestion and my implementation are good patterns to initialise components in Go. I use and like both depending on the situation.

I don't see it as a problem having unexported fields for options; keep in mind most standard libraries are using this pattern.

The downside of the lazy init approach is that we have to call .init in every exported method. The with approach also has an overhead of as well but is safer indeed.

lvrach · 2022-11-28T09:07:48Z

It would be nice before merging it to use it in at least one test :)

@atzoum I would added it here If @achettyiitr likes it.

I don't want to couple these changes in the same PR, given it addressing a different audience and they have different priorities.

fracasula

@lvrach I replied to the "redundant check" comment. if it makes sense please make the changes. overall again, it looks good. thanks for the PR 👍

achettyiitr · 2022-11-28T15:09:21Z

It would be nice before merging it to use it in at least one test :)

@atzoum I would added it here If @achettyiitr likes it.

I don't want to couple these changes in the same PR, given it addressing a different audience and they have different priorities.

Thanks, @lvrach for the PR.

chore: introduce memory stats for testing

73fc1f6

github-actions bot added server-team with tests labels Nov 24, 2022

chore: keep record of stats values/durations

0d8d0ed

lvrach marked this pull request as ready for review November 24, 2022 10:29

lvrach requested review from achettyiitr, atzoum and fracasula November 24, 2022 11:52

$fracasula$

fracasula reviewed Nov 24, 2022

View reviewed changes

atzoum approved these changes Nov 25, 2022

View reviewed changes

lvrach requested a review from fracasula November 28, 2022 09:27

lvrach changed the title ~~chore: introduce memory stats for testing~~ chore: introduce in-memory stats for testing Nov 28, 2022

refactor: use thread safe methods to get values & durations

ce8226b

$fracasula$

fracasula approved these changes Nov 28, 2022

View reviewed changes

refactor: use New creation pattern

d755518

achettyiitr approved these changes Nov 28, 2022

View reviewed changes

chore: fmt

0daa80e

lvrach force-pushed the chore.memstats branch from 3ee8df1 to 0daa80e Compare November 28, 2022 15:28

Merge branch 'master' into chore.memstats

7f0c52e

lvrach merged commit 6ac8c31 into master Nov 28, 2022

lvrach deleted the chore.memstats branch November 28, 2022 21:30

achettyiitr mentioned this pull request Nov 29, 2022

feat(warehouse): added support for warehouse column count limit #2723

Merged

1 task

This was referenced Dec 1, 2022

chore: release 1.3.0 #2749

Closed

chore: prerelease 1.4.0-rc.1 #2750

Merged

chore: release 1.4.0 #2752

Merged

chore: prerelease 1.4.0-rc.1 #2757

Closed

chore: prerelease 1.4.0-rc.2 #2759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: introduce in-memory stats for testing #2735

chore: introduce in-memory stats for testing #2735

lvrach commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading

$@fracasula$ fracasula left a comment •

edited

Loading

$@fracasula$ fracasula Nov 24, 2022

lvrach Nov 28, 2022

$@fracasula$ fracasula Nov 28, 2022

lvrach Nov 28, 2022

$@fracasula$ fracasula Nov 24, 2022

$@fracasula$ fracasula Nov 24, 2022

lvrach Nov 28, 2022

$@fracasula$ fracasula Nov 24, 2022

atzoum left a comment

atzoum Nov 25, 2022 •

edited

Loading

lvrach Nov 28, 2022

lvrach commented Nov 28, 2022

$@fracasula$ fracasula left a comment

achettyiitr commented Nov 28, 2022

	if m.Type != stats.CountType {
	panic("operation Increment not supported for measurement type:" + m.Type)
	}


		var _ stats.Measurement = (*Measurement)(nil)

		type Store struct {

chore: introduce in-memory stats for testing #2735

chore: introduce in-memory stats for testing #2735

Conversation

lvrach commented Nov 24, 2022 • edited Loading

Description

Motivation

Proposed solution

Limitations

Notion Ticket

Security

codecov bot commented Nov 24, 2022 • edited Loading

Codecov Report

fracasula left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atzoum left a comment

Choose a reason for hiding this comment

atzoum Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvrach commented Nov 28, 2022

fracasula left a comment

Choose a reason for hiding this comment

achettyiitr commented Nov 28, 2022

lvrach commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading

$@fracasula$ fracasula left a comment •

edited

Loading

atzoum Nov 25, 2022 •

edited

Loading

$@fracasula$ fracasula left a comment