Conversation
added 16 commits
May 6, 2023 00:12
We do not care about inbound dependency violations – only outbound.
When we say "dependency violations," we always mean outbound. This makes it more concise and removes the unnecessary and confusing term "outbound."
When we say privacy violations, we always mean "inbound." The distinction is confusing since our area of focus should always be inbound privacy violations.
Outbound privacy violations are not particularly interesting. While it may be a good idea to solve, the area of focus should always be inbound privacy violations first.
…ly count inbound violations It was a bit confusing that previously, by_package.dependency violations counted a package's outbound AND inbound violations. This creates double counting of violations AND we should only care about outbound dependency violations anyways. `by_package.outbound_privacy_violations.per_package` was also a bit of a misnomer, because we were actually showing the results for INBOUND violations into the selected package. This was mostly an ergonomic thing (because this was the view of per package we wanted, and we didn't want folks to have to select their pack in "to_package." Instead, we should rename this by_package.privacy_violations.per_package, which shows the inbound privacy violations for the selected package, and we should rename to_package to "other_package" to_team => other_team and to_package => other_package The idea here is that this template var is no longer used to represent "directionality" of a violation. Instead, it's used to represent the other team or package involved in a violation we care about. For example, with by_package.privacy_violations.per_package, the "other_package" is the package that is doing the violating. However for by_package.dependency_violations.per_package, the "other_package" is the package being violated. We actually don't care about the directionality in this context. We also actually don't care about who the other is. That is, it would never make sense to view the pack stats dashboard and look at "other_package" or "other_team." They're only to be used to display a widget of violation pairs, but we can actually remove them from the stats dashboard entirely!
We already ahve this in the form all_packages.packwerk_checkers.enforce_x.strict|true|false.count We can always sum the maximum of these values to get count of all true or strict.
We don't use this, and it's superceded by other metrics that count total number of violations. We can always have a DD query that checks for violations = 0.
We can simply call these "dependencies" of a package to be concise, as that is what is in the package.yml
I think "depended on" is clearer than "inbound explicit dependencies"
We are going to move to using the currently supported public API.
alexevanczuk
commented
May 6, 2023
| modularization.all_packages.with_violations.count | ||
| modularization.by_package.all_files.count | ||
| modularization.by_package.dependency_violations.count | ||
| modularization.by_package.inbound_dependency_violations.count |
Contributor
Author
There was a problem hiding this comment.
I added this test on main. Its purpose is to show the unique metric names that pack_stats emits.
More info at 4603dce#diff-8977ec6cc28cca4acf977af397d9b00d83eeede5b1a33de4d19d2850e0c0bd71R594
You can look at changes to these test expectations to get a summary of which metrics are being changed how.
You can look at changes to the other specific metric tests to see exactly how definitions are being changed.
added 3 commits
May 7, 2023 16:15
I had a bug in the way that I got the team-to-team violations. I fixed the bug by clarifying the variable names within the class and then fixing what violations were selected.
I didn't like the use of the preposition "per" in "per_package" and "per_team." The fact that it was something other than "by" (which was the previous part of the metric, i.e. by_package, by_team) suggested there was something different about it. In reality, we are just grouping by package and by other package. Using the same preposition made it clearer to me (and hopefully the reader) what the metric was for, and the fact that it matched up to the tag name makes extra sense.
1360800 to
8336950
Compare
added 2 commits
May 7, 2023 23:07
This is similar to the same bug detected in by_team.violations.by_other_team
professor
approved these changes
May 8, 2023
This was referenced May 8, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a pretty meaningful revamp of metrics. This is probably best viewed commit by commit, since I created a description of each commit (that is not viewable unless viewed commit by commit).
Locally I sent these metrics to Datadog with all metrics prefixed with
test.to ensure that things look good.You can see the resulting dashboard here: https://app.datadoghq.com/dashboard/s3q-cb3-bed?tpl_var_violation_type%5B0%5D=dependency&tpl_var_violation_type%5B1%5D=privacy&from_ts=1683392313276&to_ts=1683478713276&live=false
This dashboard will replace both the executive summary and per-package-per-team modularization dashboards to create a single, concise, unifying dashboard.
The general idea is:
Commits