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

Aqa metrics/#5121 #972

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

YosiElias
Copy link
Contributor

Fixes: #5121

Added:

  • Test Failures By Machine
  • Job Success Rate
  • Manual Rerun Targets Involved
  • Manual Rerun Needed (Include link to the relevant jobs - screenshot attached)
    Moved 'Tests Success Rate' to be under 'Test Summary'

Test Results Table:
image

When clicking on the 'Manual Rerun Needed' link:
image

Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for eclipsefdn-adoptium-trss canceled.

Name Link
🔨 Latest commit aa52396
🔍 Latest deploy log https://app.netlify.com/sites/eclipsefdn-adoptium-trss/deploys/67a4bc3459ac53000898f9f9

Yosi Elias and others added 7 commits February 5, 2025 16:33
Signed-off-by: yelias <[email protected]>
Signed-off-by: YosiElias <[email protected]>
Signed-off-by: yelias <[email protected]>
Signed-off-by: YosiElias <[email protected]>
Signed-off-by: yelias <[email protected]>
Signed-off-by: YosiElias <[email protected]>
Signed-off-by: YosiElias <[email protected]>
@smlambert
Copy link
Contributor

Thanks @YosiElias, we also need Liav to sign the ECA (as you had to recently, details at https://adoptium.net/docs/eca-sign-off/).

@YosiElias YosiElias marked this pull request as draft February 6, 2025 09:42
Signed-off-by: YosiElias <[email protected]>
@YosiElias YosiElias marked this pull request as ready for review February 6, 2025 13:44
@smlambert smlambert requested a review from llxia February 11, 2025 16:05
@llxia
Copy link
Contributor

llxia commented Feb 13, 2025

This PR will impact the performance of the Grid View page as it contains large/inefficient MongoDB queries. Since we are already investigating existing performance issues, adding this change could further complicate the situation.

From a user’s perspective, Test Failures By Machine and Additional Metrics are more relevant to PMs. Given that the Grid View is primarily used for daily triage, I strongly recommend adding these details to a separate page.

Comment on lines +203 to +214
getSpecificAggregation(type, buildNameRegex, query) {
switch (type) {
case 'totals':
return this.getTotalsAggregation(query, buildNameRegex);
case 'rerunDetails':
return this.getRerunDetailsAggregation();
case 'jobsDetails':
return this.getJobsDetailsAggregation();
default:
throw new Error(`Unknown type: ${type}`);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

getSpecificAggregation() calls different functions based on the switch type. Each function has its own mandate and purpose. I do not think we should create getSpecificAggregation() and use the switch case here.

If you really want to utilize the common code in testResultsBaseAggregation(), pass specificAggregation in testResultsBaseAggregation().

Comment on lines +346 to +377
getJobsDetailsAggregation() {
return [
{
$addFields: {
job_success_rate: {
$round: [
{
$multiply: [
{
$divide: [
{
$size: {
$filter: {
input: "$childBuilds",
as: "build",
cond: { $eq: ["$$build.buildResult", "SUCCESS"] }
}
}
},
{ $size: "$childBuilds" }
]
},
100
]
},
2
]
}
}
},
{ $unset: ['childBuilds'] }
];
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do calculations in JS, your code should be much simpler.

as: 'build',
cond: {
$and: [
{ $in: ['$$build.buildResult', ['UNSTABLE', 'FAILED', 'ABORTED']] },
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think there is FAILED state in buildResult. It should be FAILURE.

},
{ $unset: ['childBuilds', 'manual_rerun_needed_list'] }
];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is complex to review and test. Could you write a pipeline query?

Key Performance Improvements

  • Avoids multiple $filter operations by pre-filtering failed builds.
  • Replaces $reduce with $sum in $lookup, making it more efficient.
  • Minimizes $regexMatch usage, reducing processing time.
  • Efficient regex generation using $group, avoiding multiple conditionals.

@llxia
Copy link
Contributor

llxia commented Feb 18, 2025

Here are some ways to break this down:

  • create a separate page for the metrics. It can be linked from the Grid view.
  • separate PRs for query improvements

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.

EPIC: Provide AQA Test metrics per release
3 participants