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

Move the stats package content to metrics package #2433

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Mar 10, 2022

What this PR is about

Until now, we had two separate stats and metrics packages. Although
their content was related, some components were living in one, some in
the other. It leads to difficulties when having to work with both, and
made cyclic dependencies easy to run into.

To simplify our workflow, and facilitate further developments, this
commit moves the content of the stats package in the top-level
metrics package. As of this commit, the stats package is removed,
and all dependencies using it have been updated to use the metrics
package instead.

Why?

This allows to easily avoid the cyclic dependency issues I've been running into whenever I attempt to touch/refactor the original thresholds code. This also builds upon the refactors made by @na-- in #cleanup-7, and leads them to the finish line 🏁. Moreover, the code between metric and stats as it was laid out before made it sometimes hard to reason about one or the other. I do believe having all these types/concepts in a single place are advantageous.

How to review

🚨 I know, this PR is HUGE 🚨 But, the good news is, you don't even need to look at the changes 🦖. It consists only in moving code around, updating imports, and replacing stats. calls into metrics..

However, I do think it'd be valuable if you could check out the branch, and look at the new structure of the top-level metrics package. I took the liberty to move some types in dedicated files for clarity, but that's really most of the actual changes involved in this.

Dependencies

@oleiade oleiade requested review from codebien and na-- March 10, 2022 16:08
@oleiade oleiade self-assigned this Mar 10, 2022
@codebien
Copy link
Contributor

@oleiade Out of curiosity, why stats => to metrics and not the opposite?

@oleiade
Copy link
Member Author

oleiade commented Mar 11, 2022

@codebien that's a good question. While discussing this, it emerged that the idea to move things this way was already hanging around. @na-- had already started, so this PR just follows the movement. Besides, I do think that most of the types that have been moved are really related to Metrics, and that it's a better name, but that's really just me 🦖

@na--
Copy link
Member

na-- commented Mar 11, 2022

Yeah, in #2426 I needed to add a MetricsEngine somewhere and I wanted for that to not be /lib... 😅 We already had /lib/metrics for the Registry, so:

  1. move the /lib/metrics to /metrics directly, to start reducing the things in /lib
  2. it felt natural to put it the MetricsEngine under /metrics/engine... (I'd have put it under /metrics directly, but that would cause an import loop 😞 though there is a chance for that to disappear with a bit more refactoring 🤞 )
  3. move /stats to /metrics and refactor some of the things there (e.g. completely remove stats.New() and only allow metric creation through a Registry, which btw is one change request I have for this PR after I skimmed it)

At the end of that, we'll be left with a nice self-contained /metrics package that's reasonably clean and technical debt free 🎉 😅 It could have been stats, but the metrics name seems more well suited, esp. for metrics.Registry

@oleiade oleiade mentioned this pull request Mar 14, 2022
@oleiade
Copy link
Member Author

oleiade commented Mar 14, 2022

@na-- #2442 addresses the removal of stats.New, and targets master. Once merged we should rebase #cleanup-7 on master (and update #2433 accordingly).

@oleiade
Copy link
Member Author

oleiade commented Mar 17, 2022

This PR has been rebased on #2442, which is in turn based on #2426. As far as I can tell, test failures are transitives, and will be automagically fixed as soon as #2426 addresses them.

metrics/sample_test.go Outdated Show resolved Hide resolved
@oleiade oleiade added this to the v0.38.0 milestone Mar 23, 2022
@na-- na-- force-pushed the cleanup-7 branch 3 times, most recently from 58fca82 to 8276246 Compare March 29, 2022 09:13
Base automatically changed from cleanup-7 to master March 29, 2022 13:26
@oleiade oleiade force-pushed the refactor/move_stats_to_metrics branch 6 times, most recently from 9e87a49 to 1756566 Compare March 29, 2022 15:57
@oleiade oleiade force-pushed the refactor/move_stats_to_metrics branch from 1756566 to 8ea9ce4 Compare March 29, 2022 16:11
Until now, we had two separate `stats` and `metrics` packages. Although
their content was related, some components were living in one, some in
the other. It lead to difficulties when having to work with both, and
made cyclic dependencies really easy to run into.

To simplify our workflow, and facilitate further developments, this
commit moves the content of the `stats` package in the top-level
`metrics` package. As of this commit, the `stats` package is removed,
and all dependencies using it have been updated to use the `metrics`
package instead.
@oleiade oleiade force-pushed the refactor/move_stats_to_metrics branch from 8ea9ce4 to 766afc4 Compare March 29, 2022 16:27
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM 🙇‍♂️

@oleiade oleiade merged commit 59a8883 into master Mar 30, 2022
@oleiade oleiade deleted the refactor/move_stats_to_metrics branch March 30, 2022 11:43
imiric pushed a commit to grafana/xk6-browser that referenced this pull request May 5, 2022
This is a fix for a breaking change introduced in k6 v0.38.0[1].

[1]: grafana/k6#2433
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants