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

feat(core): scoring data customizable #2353

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

PagoNxt-Trade
Copy link

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

This PR add a command line parameter to add a config file for results configuration which adds a scoring calculated and qualified with this results and a limit for passing

@pablonxt
Copy link

pablonxt commented Dec 1, 2022

From PagoNxt Trade, we are very excited to share this functionality with the community!

@PagoNxt-Trade PagoNxt-Trade changed the title Feature scoring data customizable feat(scoring): scoring data customizable Dec 1, 2022
@PagoNxt-Trade PagoNxt-Trade changed the title feat(scoring): scoring data customizable feat(core): scoring data customizable Dec 1, 2022
@PagoNxt-Trade PagoNxt-Trade force-pushed the feature-AT-458-scoring_data_customizable branch 4 times, most recently from 406fe0f to 2e037f5 Compare December 1, 2022 10:21
@PagoNxt-Trade PagoNxt-Trade marked this pull request as draft December 1, 2022 12:02
@PagoNxt-Trade PagoNxt-Trade marked this pull request as ready for review December 1, 2022 13:23
@PagoNxt-Trade PagoNxt-Trade force-pushed the feature-AT-458-scoring_data_customizable branch from cc4f5a9 to 35b9256 Compare December 1, 2022 13:29
@PagoNxt-Trade PagoNxt-Trade marked this pull request as draft December 1, 2022 15:53
@PagoNxt-Trade PagoNxt-Trade force-pushed the feature-AT-458-scoring_data_customizable branch 2 times, most recently from 5c0bc4b to 9d44008 Compare December 2, 2022 08:10
@PagoNxt-Trade PagoNxt-Trade marked this pull request as ready for review December 2, 2022 08:13
@PagoNxt-Trade PagoNxt-Trade force-pushed the feature-AT-458-scoring_data_customizable branch from 9d44008 to aa3cbc2 Compare December 2, 2022 08:32
@P0lip
Copy link
Contributor

P0lip commented Dec 13, 2022

Hey!
Thanks a lot for the PR. I had time off recently, and now trying to catch up with everything. I'll make sure to have a pass on it this week.

@P0lip P0lip force-pushed the develop branch 2 times, most recently from cf3ae99 to 761c65a Compare December 14, 2022 15:55
@P0lip P0lip self-requested a review December 15, 2022 19:18
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Looking pretty neat!

What do you think @mnaumanali94?

@@ -127,6 +135,11 @@ const lintCommand: CommandModule = {
description: 'path/URL to a ruleset file',
type: 'string',
},
'scoring-config': {
alias: 's',
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh this option is rather quite specific, therefore I'm not sure whether an alias for it makes that much sense.
Would just keep it without an alias for now.

scoringFile = path.join(process.cwd(), scoringFile);
}

const scoringConfig: ScoringConfig = JSON.parse(fs.readFileSync(scoringFile, 'utf-8')) as ScoringConfig;
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
const scoringConfig: ScoringConfig = JSON.parse(fs.readFileSync(scoringFile, 'utf-8')) as ScoringConfig;
const scoringConfig: ScoringConfig = JSON.parse(fs.promises.readFile(scoringFile, 'utf8'));

I'd also say we should impose some validation.
We could define a JSON Schema for the format and use Ajv for that purpose.
What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

We've got your suggestion
For the validation ... We guess that could be a duplicated validation, with type defined on cli/src/formatters/types.ts should be enough to validate input file, but, if anybody thinks like you about this, we could add this

@@ -60,6 +61,92 @@ Here you can build a [custom ruleset](../getting-started/3-rulesets.md), or exte
- [OpenAPI ruleset](../reference/openapi-rules.md)
- [AsyncAPI ruleset](../reference/asyncapi-rules.md)

## Scoring the API
Copy link
Contributor

Choose a reason for hiding this comment

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

@pamgoodrich do you want to have a quick look at the docs here?

let scoringText = '';
if (options.scoringConfig !== void 0) {
if (options.scoringConfig.customScoring !== undefined) {
spectralVersion = `${options.scoringConfig.customScoring} ${version as string}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think including a version makes much sense.
The result does not directly depend on the version of the CLI client, but on the ruleset you use, so seems like we could just not include it?

const cliui = require('cliui');
let output = '\n';
if (options.scoringConfig !== void 0) {
if (options.scoringConfig.customScoring !== void 0) {
output += `${options.scoringConfig.customScoring}${version as string}\n`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto ^

packages/cli/src/commands/lint.ts Show resolved Hide resolved
- scoringLetter : An object with key/value pairs with scoring letter and scoring percentage, that the result must be greater , for this letter
- threshold : A number with minimum percentage value to provide valid the file we are checking
- warningsSubtract : A boolean to setup if accumulate the result types to less the scoring percentage or stop counting on most critical result types
- uniqueErrors : A boolean to setup a count with unique errors or with all of them
Copy link
Contributor

Choose a reason for hiding this comment

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

We should define what a unique error would be.
Something like "An error is considered unique if its code and message have not been seen yet" or something similar.

@@ -0,0 +1,16 @@
import { IRuleResult } from '@stoplight/spectral-core';

export const uniqueErrors = (results: IRuleResult[]): IRuleResult[] => {
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
export const uniqueErrors = (results: IRuleResult[]): IRuleResult[] => {
export const getUniqueErrors = (results: IRuleResult[]): IRuleResult[] => {

if (scoring >= options.scoringConfig.threshold) {
output += chalk['green'].bold(`\u2716 PASSED!\n`);
} else {
output += chalk['red'].bold(`\u2716 NOT PASSED!\n`);
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
output += chalk['red'].bold(`\u2716 NOT PASSED!\n`);
output += chalk['red'].bold(`\u2716 FAILED!\n`);

if (scoring >= options.scoringConfig.threshold) {
output += chalk['green'].bold(`\u2716 PASSED!\n`);
} else {
output += chalk['red'].bold(`\u2716 NOT PASSED!\n`);
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
output += chalk['red'].bold(`\u2716 NOT PASSED!\n`);
output += chalk['red'].bold(`\u2716 FAILED!\n`);

@PagoNxt-Trade PagoNxt-Trade force-pushed the feature-AT-458-scoring_data_customizable branch 2 times, most recently from 47bac35 to 01004ea Compare December 20, 2022 11:07
@pablonxt
Copy link

pablonxt commented Jan 9, 2023

@pamgoodrich all your suggestions have been applied. Thanks!

@shrutiparabgoogle
Copy link

@PagoNxt-Trade This is interesting! I have some concerns about locking in a scoring mechanism, as flexibility to model different use cases and opinions is a significant component to retrieving value out of the system.

It might be helpful to get the perspective of some Apigee folks who are also working on a scoring approach via https://github.com/apigee/registry. I believe, in their case, they're allowing folks to provide their own custom CEL formulas to determine the score.

Any input @timburks, @shrutiparabgoogle, @theganyo?

This is interesting! Thanks for the mention.

I can see that the scoring approach mentioned here is making some assumptions:

  • that the score is always a percentage
  • the formula used to calculate the score is also opinionated (subtract from 100)

The approach that we have implemented in https://github.com/apigee/registry is flexible with the goal to accommodate scores for anything and not only lint results. Hence, we made use of CEL expressions so that users can define their own formula for calculating the score.

While that kind of flexibility was one of the goals for us, I can see that it can be an overkill for lint specific scoring. Couple of thoughts on the current approach:

  • Will scores always be percentages or is there a need to support a more flexible range? It might also help to add some validation while accepting a scoring config, so that users don't accidentally generate invalid scores (like negative percentages -2%?). I might have missed it if it’s already in place.
  • What is the purpose of letter scoring? (trying to understand the use case here). IIUC, it is mapping the generated percentage to a letter score, what difference does looking at a letter provide me vs looking at a percentage? Perhaps these are two different ways of conveying the same information and there can be flexibility in whether users want a lettered score or a percentage score?
  • The use of threshold and warningsSubtract flags is not very clear to me from the documentation, explicitly mentioning the formula in the docs might help gain clarity in the assumptions made.
  • If the user doesn’t provide a scoringConfig, is there a default config they can get started with?

@pablonxt
Copy link

Hi @shrutiparabgoogle

Thanks for your input. Here you have our answers.

Usually scoring is done starting with 100% and substracting a value depending on the issue severity (error, warning,...) this is a common practice for scoring. We think is a complete overkill to go for CELs

What can be a more flexible range? We see this as previous sentence (a percentage)

About the possible negative, we will run some tests to validate that it can't happen

The purpose of letter scoring is to show a nicer scoring than a raw number. Letter scoring comes as anything you like A+, A, B, C ... See qualys ssl tests results here: https://www.ssllabs.com/ssltest/

The number or percentage is required by the algorithm to be able to substract values

We can make letter scoring optional if you don't provide the configuration of it. So results will be always percentaje and optionally letter if you provide the letter scoring configuration.

The use of threshold and warningsSubtract flags is not very clear to me from the documentation, explicitly mentioning the formula in the docs might help gain clarity in the assumptions made.

threshold : A number with minimum percentage value to provide valid the file we are checking. Any scoring below this thresold will mark the API as a failure in the scoring.

warningsSubtract, we are going to change its name to onlySubtractHigherSeverityLevel and adaprt the documentation for more clarity.

onlySubtractHigherSeverityLevel : A boolean to decide if only the higher severity level who appears in the results for the API to analize, are subtracted from scoring or every severity level are subtracted from scoring.

See sample:

true

API with Errors and Warnings, only Errors substract from scoring
API with Warnings, Warnings substract from scoring

false

API with Errors and Warnings, Errors and Warnings substracts from scoring
API with Warnings, Warnings substract from scoring

If the user doesn’t provide a scoringConfig, is there a default config they can get started with?

If you don't provide scoringConfig there will be no scoring. As spectral does now.

Does this work better?

@PagoNxt-Trade PagoNxt-Trade force-pushed the feature-AT-458-scoring_data_customizable branch from 1b682b7 to aeb85a4 Compare January 25, 2023 09:03
@pablonxt
Copy link

Hi .. do you have any update? @P0lip @pamgoodrich @mnaumanali94

@P0lip
Copy link
Contributor

P0lip commented Jan 31, 2023

@mnaumanali94 ^

- scoringSubtract : An object with key/value pair objects for every result level we want to subtract percentage, with the percentage to subtract from number of results on every result type
- scoringLetter : An object with key/value pairs with scoring letter and scoring percentage, that the result must be greater, for this letter
- threshold : A number with minimum percentage value to provide valid the file we are checking. Any scoring below this thresold will mark the API as a failure in the scoring.
- onlySubtractHigherSeverityLevel : A boolean to decide if only the higher severity level who appears in the results for the API to analize, are subtracted from scoring or every severity level are subtracted from scoring.
Copy link

Choose a reason for hiding this comment

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

analize-> analyze

@pablonxt
Copy link

@mnaumanali94 do you have any update? We need this PR merged

@P0lip P0lip force-pushed the develop branch 7 times, most recently from dc9d7f4 to 44c40e2 Compare May 23, 2023 22:56
@P0lip P0lip force-pushed the develop branch 4 times, most recently from 02ec0d4 to 84faec8 Compare June 9, 2023 19:43
@P0lip P0lip force-pushed the develop branch 3 times, most recently from 9e92f34 to 6d09915 Compare September 20, 2023 18:42
@P0lip P0lip force-pushed the develop branch 3 times, most recently from dc90b7a to c22f408 Compare April 4, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants