feature(backend): Enabling Analyzer Configuration #2842

xoscar · 2023-06-30T19:23:00Z

This PR updates the architecture for plugins and rules, keeping configuring plugins more straightforward and flexible.

Changes

Decouples rules from plugins and allows for a configuration-based setup
Moves the linter out of the models to the linter directory
Updates plugins and rules interfaces to support an analyzer configuration
Moves metadata away from linter execution, leaving it to be config only
Enables configuration for error level and weight for rules

Fixes

[Analyzer] Configuration - Backend #2773

Checklist

tested locally
added new dependencies
updated the docs
added a test

Analyzer Arch Changes
https://www.loom.com/share/03e95872d383496aa81591be9d772123

Features & bug fixes
https://www.loom.com/share/0517b986f19941f3b41c5187d2344500

xoscar · 2023-06-30T19:37:24Z

server/executor/linter_runner.go


-	shouldSkip, reason := linter.ShouldSkip()
+	shouldSkip := lintResource.ShouldSkip()


The analyzer resource should make the decisions of running or not the linter as it contains the configuration

xoscar · 2023-06-30T19:37:47Z

server/linter/analyzer/analyzer_entities.go

+		Description string `json:"-"`
+	}
+
+	LinterRule struct {


This a new block, allowing users to configure rules

xoscar · 2023-06-30T19:38:51Z

server/linter/analyzer/analyzer_entities.go

@@ -40,18 +58,130 @@ func (l Linter) Validate() error {
 	}

 	for _, p := range l.Plugins {
-		if p.Name == "" {
-			return fmt.Errorf("plugin name cannot be empty")
+		plugin, ok := findPlugin(p.Id, DefaultPlugins)


Validates that the plugins and rules coming from the new resource instance match what we have in the system, this should be more flexible in the future when we implement custom analyzer profiles

xoscar · 2023-06-30T19:39:58Z

server/linter/analyzer/analyzer_entities.go

+	return !l.Enabled
+}
+
+func (l Linter) WithMetadata() (Linter, error) {


Combines the analyzer configuration (Database, CLI, FE) with the metadata stored within the repo, as it is not available for users to update (today)

xoscar · 2023-06-30T19:40:14Z

server/linter/analyzer/analyzer_metadata.go

+
+var (
+	// plugins
+	StandardsId = "standards"


Names, ids, descriptions, etc

xoscar · 2023-06-30T19:42:58Z

server/linter/analyzer/analyzer_results.go

@@ -0,0 +1,94 @@
+package analyzer


The "analyzer" per se, doesn't have results, as those belong to the linter, but having them at the linter level creates an import cycle issue because the linter imports model.Trace for rules and plugins, and at the same time, model.Run requires the linter.Results to persist the output from the analyzer.

Once the test migration changes are done, we can come back to this file and move it to the linter level and everyone will be happy.

At least is not in the models folder anymore 🤷🏾

xoscar · 2023-06-30T19:44:14Z

server/linter/linter.go

-		linter_plugin_security.NewSecurityPlugin(),
-		linter_plugin_common.NewCommonPlugin(),
-	}
+	commonPlugin = plugins.NewPlugin(


Static registration of the linter configuration, we do not have much flexibility today, but in the future, the linter runner could generate a dynamic plugin registry with custom rules and plugins defined by user profile, test or test run levels

xoscar · 2023-06-30T19:45:28Z

server/linter/plugins/plugins_entities.go

+	return p.ruleRegistry
+}
+
+func (p BasePlugin) Execute(ctx context.Context, trace model.Trace, config analyzer.LinterPlugin) (analyzer.PluginResult, error) {


All current plugins have the same execution logic, so instead of having to create a new plugin file for each, we use the default, and if needed, we can create specific plugin files implementing the Plugin interface to override execute method with custom logic

xoscar · 2023-06-30T19:46:32Z

server/linter/rules/rules_entities.go

+	return r.id
+}
+
+func (r BaseRule) Evaluate(ctx context.Context, trace model.Trace, config analyzer.LinterRule) (analyzer.RuleResult, error) {


Rules on the other hand, have very custom logic, which makes it more difficult to encapsulate in a single default method, for these, we should create an implementation of the Rule interface per rule

I would say there's no need for this BaseRule. it can be replaced by a func (r MyRule) ID() string {return someconstantID} func, satisfying the interface. This also makes the ID immutable, and that's good in this context

xoscar · 2023-06-30T19:47:16Z

server/resourcemanager/resource_manager.go

@@ -222,6 +222,15 @@ func (m *manager[T]) create(w http.ResponseWriter, r *http.Request) {
 		targetResource.Spec = m.rh.SetID(targetResource.Spec, m.config.idgen())
 	}

+	if err := targetResource.Spec.Validate(); err != nil {


I added this validate step to the resource manager, as it wasn't currently in use, not sure if I should better move this to another PR

…ration-backend

danielbdias

The code overall is ok! Good job!

One thing to consider in the future is how we can test these rules to guarantee that they are working as intended and are only sensitive to changes in the future without us knowing it.

xoscar · 2023-07-04T17:05:46Z

The code overall is ok! Good job!

One thing to consider in the future is how we can test these rules to guarantee that they are working as intended and are only sensitive to changes in the future without us knowing it.

Hey @danielbdias thanks for the review, I have added tests for the analyzer and linter critical points, and will add more for rules in the following PRs!

schoren

It looks good in general. I think we should try to embrace go's naming conventions, so it'd be great to rename all the Ids to ID.

I outlined a couple of duplicated loops and bugs, not necessarily related specifically to this work, but since we're here 🤷

Finally, more oriented to the architecutre:

I would remove the BaseRule, it's being used kind of as a base class, but it doesn't make a lot of sense here IMO.
We should try to reduce the amount of interfaces that are not strictly required. "Accept interfaces, return structs". You can leverage go's duck typing and declare interfaces where you want to avoid direct references to a package.

The rest is looking really good. I like the amount of tests that are included here!

schoren · 2023-07-04T23:36:14Z

server/linter/analyzer/analyzer_entities.go

-		Name     string `json:"name"`
-		Enabled  bool   `json:"enabled"`
-		Required bool   `json:"required"`
+		Id      string       `json:"id"`


Suggested change

Id string `json:"id"`

ID string `json:"id"`

schoren · 2023-07-04T23:38:37Z

server/linter/analyzer/analyzer_entities.go

+	return LinterPlugin{}, false
+}
+
+func findRule(Id string, rules []LinterRule) (LinterRule, bool) {


Suggested change

func findRule(Id string, rules []LinterRule) (LinterRule, bool) {

func findRule(id string, rules []LinterRule) (LinterRule, bool) {

schoren · 2023-07-04T23:40:56Z

server/linter/analyzer/analyzer_metadata.go

+
+var (
+	// plugins
+	StandardsId = "standards"


Suggested change

StandardsId = "standards"

StandardsID = "standards"

schoren · 2023-07-04T23:41:06Z

server/linter/analyzer/analyzer_metadata.go

+var (
+	// plugins
+	StandardsId = "standards"
+	CommonId    = "common"


Suggested change

CommonId = "common"

CommonID = "common"

schoren · 2023-07-04T23:43:33Z

server/linter/analyzer/analyzer_metadata.go

+	// plugins
+	StandardsId = "standards"
+	CommonId    = "common"
+	SecurityId  = "security"


for consistency, and as per Go's recommendation the casing for ID should be ID and not Id

schoren · 2023-07-05T00:07:12Z

server/linter/rules/required_attribute_rule.go

+		for _, span := range trace.Flat {
+			res = append(res, r.validateSpan(span))
+		}
+
+		for _, result := range res {
+			if !result.Passed {
+				allPassed = false
+			}
+		}


same here

Suggested change

for _, span := range trace.Flat {

res = append(res, r.validateSpan(span))

}

for _, result := range res {

if !result.Passed {

allPassed = false

}

}

for _, span := range trace.Flat {

analyzerResult := r.validateSpan(span)

res = append(res, analyzerResult)

if !analyzerResult.Passed {

allPassed = false

}

}

schoren · 2023-07-05T00:10:27Z

server/linter/rules/rules_entities.go

+	return r.id
+}
+
+func (r BaseRule) Evaluate(ctx context.Context, trace model.Trace, config analyzer.LinterRule) (analyzer.RuleResult, error) {


I would say there's no need for this BaseRule. it can be replaced by a func (r MyRule) ID() string {return someconstantID} func, satisfying the interface. This also makes the ID immutable, and that's good in this context

schoren · 2023-07-05T00:24:08Z

server/linter/rules/rules_registry.go

+	Evaluate(context.Context, model.Trace, analyzer.LinterRule) (analyzer.RuleResult, error)
+}
+
+type RuleRegistry interface {


Who depends on this interface? can the dependees directly reference a struct? What benefits do we get from this interface?

If a client depends on this interface, it will be requiring this package. Given that we only support one implementation of the RuleRegistry, and it's in memory without any dependencies, doesn't look like we would need to mock it, for example, or replace it by another implementation.

Makes sense, I think the best case is just to remove the interface

schoren · 2023-07-05T00:25:07Z

server/resourcemanager/resource_manager.go

@@ -255,6 +255,15 @@ func (m *manager[T]) doCreate(w http.ResponseWriter, r *http.Request, encoder En
 		specs = m.rh.SetID(specs, m.config.idgen())
 	}

+	if err := specs.Validate(); err != nil {


schoren · 2023-07-05T00:27:08Z

server/resourcemanager/resource_manager.go

@@ -288,6 +297,15 @@ func (m *manager[T]) upsert(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	if err := targetResource.Spec.Validate(); err != nil {


this is already handled on doCreate, and if you move this to happen within the doUpdate method, we can have both cases covered and remove the need for extra work here

xoscar · 2023-07-05T16:08:14Z

Hey @schoren thanks for the review it was really insightful! I think I addressed all of your comments and updated the code accordingly, let me know if there is something else I could do

schoren

Thanks @xoscar! this looks very consistent now. I would strongly suggest to remove the remaining duplicated loops (here and here) and remove the BaseRule, since it's now unused (I think?)

Other than that, LGTM

* feature(backend): Enabling Analyzer Configuration (#2842) * feature: Enabling Analyzer Configuration * feature: Enabling Analyzer Configuration * cleanup and improvements * cleanup and improvements * improvements, upgrades and simplification * improvements, upgrades and simplification * adding tests for the analyzer and linter * fixing linter runner * updates and fixes based on PR review comments * removing unnecessary loop * feature(frontend): Analyzer Configuration (#2850) * feature: Enabling Analyzer Configuration * feature: Enabling Analyzer Configuration * cleanup and improvements * cleanup and improvements * improvements, upgrades and simplification * improvements, upgrades and simplification * adding tests for the analyzer and linter * fixing linter runner * feature(frontend): Analyzer Configuration * feature(frontend): Analyzer Configuration * updates and fixes based on PR review comments * clean up changes * fix(frontend): fix analyzer score styles (#2860) * feature(cli): Analyzer Configuration (#2873) * remove file with conflict * fix disabled rule logic --------- Co-authored-by: Jorge Padilla <[email protected]>

xoscar added 6 commits June 29, 2023 16:38

feature: Enabling Analyzer Configuration

570d5a9

feature: Enabling Analyzer Configuration

0f31de3

cleanup and improvements

770f1c8

cleanup and improvements

e780044

improvements, upgrades and simplification

dbadaca

improvements, upgrades and simplification

40a59ea

xoscar changed the base branch from main to feat/analyzer-configuration June 30, 2023 19:24

xoscar changed the title ~~Feat/analyzer configuration backend~~ feature: Enabling Analyzer Configuration - Backend Jun 30, 2023

xoscar self-assigned this Jun 30, 2023

xoscar added the backend label Jun 30, 2023

xoscar commented Jun 30, 2023

View reviewed changes

xoscar marked this pull request as ready for review June 30, 2023 19:47

xoscar requested review from danielbdias, jorgeepc, schoren and mathnogueira June 30, 2023 19:48

Merge branch 'feat/analyzer-configuration' into feat/analyzer-configu…

84377cc

…ration-backend

danielbdias approved these changes Jul 4, 2023

View reviewed changes

adding tests for the analyzer and linter

206ac78

fixing linter runner

e7e3d02

schoren suggested changes Jul 5, 2023

View reviewed changes

updates and fixes based on PR review comments

8d153cb

xoscar requested a review from schoren July 5, 2023 16:08

xoscar changed the title ~~feature: Enabling Analyzer Configuration - Backend~~ feature(backend): Enabling Analyzer Configuration Jul 5, 2023

schoren approved these changes Jul 5, 2023

View reviewed changes

removing unnecessary loop

ab699cd

xoscar merged commit bc5c6cb into feat/analyzer-configuration Jul 5, 2023

xoscar deleted the feat/analyzer-configuration-backend branch July 5, 2023 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(backend): Enabling Analyzer Configuration #2842

feature(backend): Enabling Analyzer Configuration #2842

xoscar commented Jun 30, 2023 •

edited

Loading

xoscar Jun 30, 2023

xoscar Jun 30, 2023

xoscar Jun 30, 2023

xoscar Jun 30, 2023

xoscar Jun 30, 2023

xoscar Jun 30, 2023

xoscar Jun 30, 2023

xoscar Jun 30, 2023

xoscar Jun 30, 2023

schoren Jul 5, 2023

xoscar Jun 30, 2023

danielbdias left a comment

xoscar commented Jul 4, 2023

schoren left a comment •

edited

Loading

schoren Jul 4, 2023

schoren Jul 4, 2023

schoren Jul 4, 2023

schoren Jul 4, 2023

schoren Jul 4, 2023

schoren Jul 5, 2023

schoren Jul 5, 2023

schoren Jul 5, 2023

xoscar Jul 5, 2023

schoren Jul 5, 2023

schoren Jul 5, 2023

xoscar commented Jul 5, 2023

schoren left a comment


		shouldSkip, reason := linter.ShouldSkip()
		shouldSkip := lintResource.ShouldSkip()

	func findRule(Id string, rules []LinterRule) (LinterRule, bool) {
	func findRule(id string, rules []LinterRule) (LinterRule, bool) {

feature(backend): Enabling Analyzer Configuration #2842

feature(backend): Enabling Analyzer Configuration #2842

Conversation

xoscar commented Jun 30, 2023 • edited Loading

Changes

Fixes

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbdias left a comment

Choose a reason for hiding this comment

xoscar commented Jul 4, 2023

schoren 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xoscar commented Jul 5, 2023

schoren left a comment

Choose a reason for hiding this comment

xoscar commented Jun 30, 2023 •

edited

Loading

schoren left a comment •

edited

Loading