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

Refactor group and check to not be tightly coupled #3750

Merged
merged 4 commits into from
May 29, 2024

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented May 21, 2024

What?

Group and check were tightly coupled between them and also to the end of test summary and the REST API.

This change the tight coupling is removed. Group and check no longer interact with each other while being run.

In order to keep the same end of test summary and REST API behavior the code makes the same aggregation it used to be but only based on the metrics emitted by check and group.

There are several breaking changes still in it as a bunch of things are no longer useful, and technically not implementable if we want to remove the tight coupling.

The only one that is relevant though is that
lib.State no longer has Group.

There are 2 extensions using that and both of them use it to use the "magic" that tight group and check together.

Why?

The previous code apart from having problems code smell wise, meant that you can't implement this functionality in JS.

Which also means that they act very strangely especially as if you do not look closely it just seems like they set a few tags and emit metrics.

While working on #2728 it became apparent that it is unlikely we will get a perfect solution and one of the big problems is that you can't even experiment without having to write a fairly complicated async go code.

So this also makes that issue a bit more actionable. And definitely a thing that now people can have not perfect or even 80%, but still workable for them solutions.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#2869

Group and check were tightly coupled between them and also to the end of
test summary and the REST API.

This change the tight coupling is removed. Group and check no
longer interact with each other while being run.

In order to keep the same end of test summary and REST API behavior the
code makes the same aggregation it used to be but *only* based on the
metrics emitted by `check` and `group`.

There are several breaking changes still in it as a bunch of things are
no longer useful, and technically not implementable if we want to remove
the tight coupling.

The only one that is relevant though is that
`lib.State` no longer has `Group`.

There are 2 extensions using that and both of them use it to use the
"magic" that tight `group` and `check` together.

Fixes #2869
@mstoykov mstoykov added refactor breaking change for PRs that need to be mentioned in the breaking changes section of the release notes labels May 21, 2024
@mstoykov mstoykov added this to the v0.52.0 milestone May 21, 2024
@mstoykov mstoykov requested a review from a team as a code owner May 21, 2024 15:47
@mstoykov mstoykov requested review from oleiade and joanlopez and removed request for a team May 21, 2024 15:47
Comment on lines +41 to +55

// GroupSummaryDescription is the description of the GroupSummary used to identify and ignore it
// for the purposes of the cli descriptions.
const GroupSummaryDescription = "Internal Group Summary output"

// GroupSummary is an internal output implementation that facilitates the aggregation of
// group and check metrics for the purposes of the end of test summary and the REST API
type GroupSummary struct {
group *Group // TODO(@mstoykov): move the whole type outside of lib later
ch chan []metrics.SampleContainer
stopped chan struct{}
logger logrus.FieldLogger
}

// NewGroupSummary returns new GroupSummary ready to be started.
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 kind of wanted to move the whole Group, Check and GroupSummary somewhere else, but then I decided that is quite a big change and will make the review even harder.

It also turned out to not be easy.

So if we have any good places to move them I would prefer to do them in a separate PR.

Copy link
Contributor

@codebien codebien May 21, 2024

Choose a reason for hiding this comment

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

Ideally, I would use this opportunity to move them to startup an internal package and move GroupSummary there. This is my unique request, mostly to reflect what the comment is mentioning.

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 am not against this - but that will create a much bigger change that will make following what happens ever harder. So I will prefer to keep it in a separate PR.

Not certain internal is needed as well so 🤷 on that particular part. But it is probably a good usage as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, especially with the v1.0 on the road, would be great to start considering hiding internal details behind an internal package, to reduce the public surface ( 👋🏻 say hello to Hyrum's Law).

That said, I fully agree on doing so on a separate PR, to avoid making this even harder to read, follow and review.

Comment on lines -45 to -47
// Current group; all emitted metrics are tagged with this.
Group *Group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will break 2 extensions:

xk6-fasthttp/checks.go
41:     check, err := state.Group.Check("check status is " + strconv.FormatInt(int64(wantStatus), 10))

xk6-g0/g0/addon/assert.go
67:     check, err := state.Group.Check(name)

I don't really think that keeping this compatible is possible, and even more importantly warranted.

As you can see both extensions basically hit exactly the same issue with this being the way to create a new check instead of just emitting the metrics.

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2024

Codecov Report

Attention: Patch coverage is 83.72093% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 70.83%. Comparing base (153e0e5) to head (f408082).
Report is 3 commits behind head on master.

Current head f408082 differs from pull request most recent head ba596ca

Please upload reports for the commit ba596ca to get more accurate results.

Files Patch % Lines
lib/test_state.go 73.91% 6 Missing and 6 partials ⚠️
lib/models.go 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3750      +/-   ##
==========================================
- Coverage   70.85%   70.83%   -0.02%     
==========================================
  Files         289      290       +1     
  Lines       21159    21191      +32     
==========================================
+ Hits        14992    15011      +19     
- Misses       5210     5215       +5     
- Partials      957      965       +8     
Flag Coverage Δ
ubuntu 70.75% <83.72%> (-0.05%) ⬇️
windows 70.68% <83.72%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +100 to +101

if sample.Metric.Name != "checks" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if sample.Metric.Name != "checks" {
// In case of checks we need to also generate the check and add it values. Otherwise we are done.
if sample.Metric.Name != "checks" {

groupTag, ok := state.Tags.GetCurrentValues().Tags.Get("group")
require.True(t, ok)
assert.Equal(t, groupTag, root.Name)
assert.Equal(t, groupTag, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert.Equal(t, groupTag, "")
assert.Equal(t, groupTag, lib.RootGroupPath)

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 actually purposefully left it here not using the constant to catch it if ever changes - I think there is one more place where this is done in the tests as well.

I am not particularly attached to this, but as changing this constant will be a breaking change I kind of prefer if changing it breaks a few tests for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right, sometimes you used "", sometimes the constant.

but as changing this constant will be a breaking change I kind of prefer if changing it breaks a few tests for sure

I fully agree with end goal, it was me just probably confused about where and for which tests it makes more sense to use the constant and for which not. Let's keep it as-is.

That said, as I had previous negative experiences with this kind of tradeoff: "de-coupling" the test from the constant, to notice any change, would make sense to also leave a comment together with the constant, stating that explicitly, for the ourselves of the future, who might forget/omit it by just updating the tests anyway?

joanlopez
joanlopez previously approved these changes May 23, 2024
Copy link
Contributor

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

Looks good despite of the minor comments/suggestions!

Co-authored-by: Joan López de la Franca Beltran <[email protected]>
joanlopez
joanlopez previously approved these changes May 27, 2024
Copy link
Contributor

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

Approved! ✅ Although I guess we need a to reach a consensus around the breaking changes to move forward. Nevertheless, I don't see many alternatives at this point.

oleiade
oleiade previously approved these changes May 27, 2024
Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 👏🏻

I concur with what has been said about starting an internal package too, as it's inline with our ongoing plans for v1.

@mstoykov mstoykov linked an issue May 28, 2024 that may be closed by this pull request
lib/models.go Show resolved Hide resolved
// GroupSummary is an internal output implementation that facilitates the aggregation of
// group and check metrics for the purposes of the end of test summary and the REST API
type GroupSummary struct {
group *Group // TODO(@mstoykov): move the whole type outside of lib later
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
group *Group // TODO(@mstoykov): move the whole type outside of lib later
group *Group // TODO(@mstoykov): move the whole type outside of lib later

Where do you plan to move it? Under output? It would like to have a suggestion directly in the code so we have shared knowledge on ideas/plans.


// AddMetricSamples is part of the output.Output interface
func (gs *GroupSummary) AddMetricSamples(samples []metrics.SampleContainer) {
gs.ch <- samples
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we can serve the rate expected and we don't block here? Is the buffer size of the channel enough?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor group() and check() tight coupling with the VU.State
6 participants