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

fix: acceptance rate now uses suggestion/acceptance count #2170

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Zarthus
Copy link

@Zarthus Zarthus commented Dec 11, 2024

Fix acceptance rate calculation

We are migrating from https://github.com/github-copilot-resources/copilot-metrics-viewer/issues to the Backstage plugin, and noticed one critical difference in our metrics: The "Acceptance Rate Average".

The copilot plugin reports acceptance rate incorrectly, using the lines metrics rather than acceptance/suggestion count. It seems like the same code slightly later down the line does it correctly (minus the rate function):

const overallMetrics = {
acceptanceRate: calculateAcceptanceRate(metrics),
suggestionsCount:
metrics.length > 0
? sumProperty(metrics, 'total_suggestions_count')
: 'N/A',
acceptancesCount:
metrics.length > 0
? sumProperty(metrics, 'total_acceptances_count')
: 'N/A',
linesAccepted:
metrics.length > 0 ? sumProperty(metrics, 'total_lines_accepted') : 'N/A',
};

The GitHub API will respond with the following:

[
  {
    "total_suggestions_count": 600,
    "total_acceptances_count": 200,
    "total_lines_suggested": 2500,
    "total_lines_accepted": 500,
    ...

In this case, we expect acceptance rate to be 33%, not 20%, as the Backstage plugin reports. This is because copilot can generate several lines worth of suggestions, and one accept will shoot up (or down) the average significantly, whereas in total_*_count this is just a +1 to the value. Making it more reliable to report as an acceptance rate as it directly is the number of accepts/rejects, and is not influenced by the size of the suggestion.

For reference, here is how copilot-metrics-viewer does it: https://github.com/github-copilot-resources/copilot-metrics-viewer/blob/223cb4fc9f73bd6e7263f8246d9a701aca1983cf/src/components/MetricsViewer.vue#L276-L280

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Dec 11, 2024

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-copilot workspaces/copilot/plugins/copilot minor v0.4.0

@Zarthus Zarthus marked this pull request as ready for review December 11, 2024 12:10
@Zarthus Zarthus requested a review from a team as a code owner December 11, 2024 12:10
@Zarthus Zarthus requested a review from nickboldt December 11, 2024 12:10
@Zarthus
Copy link
Author

Zarthus commented Dec 17, 2024

@nickboldt if you have some time please :)

@@ -0,0 +1,5 @@
---
'@backstage-community/plugin-copilot': minor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a patch? Just curious if intentionally minor

Copy link
Author

@Zarthus Zarthus Dec 21, 2024

Choose a reason for hiding this comment

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

I was doubting that myself as well, so visually the change is big - we are suddenly reporting an entirely different number in an existing field and anyone consuming that number should properly communicate that to their user base upon upgrading.

Id be okay with a patch too, i think there's rationales for both sides. (the nature of the bug fix changes the behaviour of an existing feature significantly, which will not be "backwards compatible" visually - but also does not require any change or action from the people depending on the software)

@awanlin awanlin requested a review from BethGriggs January 3, 2025 13:56
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.

3 participants