Skip to content

fix: label metrics overcounting for closed issues when labels removed before closure #571

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

Merged
merged 5 commits into from
Aug 18, 2025

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 15, 2025

Problem

The get_label_metrics function was overcounting time spent in labels for closed issues when those labels were removed before the issue was closed.

Example scenario:

  • Issue created: day 0
  • Label added: day 5
  • Label removed: day 10
  • Issue closed: day 15

Expected result: 5 days (time label was actually applied)
Actual result: 20 days (incorrect - adds full issue duration)

Root Cause

The closed issue branch in get_label_metrics adds the full (closed_at - created_at) duration for any label that was ever applied, without checking if the label was still active when the issue closed:

if issue.state == "closed":
    # This incorrectly adds time for ALL labels that were ever applied
    label_metrics[label] += datetime.fromisoformat(issue.closed_at) - datetime.fromisoformat(issue.created_at)

The open issue branch already had the correct guard logic to skip labels whose last event was "unlabeled", but this guard was missing from the closed issue branch.

Solution

Added the same guard condition that exists in the open issue branch to the closed issue branch:

if issue.state == "closed":
    # Only add the final span if the label was still applied at closure
    if label_last_event_type.get(label) != "labeled":
        continue
    label_metrics[label] += datetime.fromisoformat(issue.closed_at) - datetime.fromisoformat(issue.created_at)

This ensures we only add the closure timespan for labels that were still active when the issue was closed.

Changes

  1. Bug fix: Added guard condition to check label_last_event_type in closed issue branch
  2. Typo fix: Corrected 'unlabled' → 'unlabeled' in comment
  3. Tests: Added comprehensive test cases covering both scenarios:
    • Label removed before closure (reproduces the bug)
    • Label remains applied through closure (ensures existing functionality works)

Verification

  • New test initially failed with 20 days vs expected 5 days, confirming the bug
  • After fix, test passes with correct 5-day calculation
  • All existing tests continue to pass (116/116)
  • Manual validation confirms the exact problem scenario now works correctly

Backward Compatibility

No interface changes - this only reduces overcounted values to their correct amounts.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: zkoppert <6935431+zkoppert@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Fix overcounting label time for closed issues in get_label_metrics Fix label metrics overcounting for closed issues when labels removed before closure Aug 15, 2025
@Copilot Copilot AI requested a review from zkoppert August 15, 2025 22:35
Copilot finished work on behalf of zkoppert August 15, 2025 22:35
@github-actions github-actions bot added the fix label Aug 15, 2025
@zkoppert zkoppert changed the title Fix label metrics overcounting for closed issues when labels removed before closure fix: label metrics overcounting for closed issues when labels removed before closure Aug 15, 2025
@zkoppert
Copy link
Member

@copilot can you generate 3 more test cases like the ones you've added that test when labels are applied at creation and also test different time spans?

…ls and varied timeframes

Co-authored-by: zkoppert <6935431+zkoppert@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 15, 2025

@copilot can you generate 3 more test cases like the ones you've added that test when labels are applied at creation and also test different time spans?

I've added 3 additional test cases as requested:

  1. Label applied at creation and removed before closure - tests a 7-day span where label is applied at issue creation and removed before closure
  2. Label applied at creation and remains through closure - tests a 30-day span where label stays applied for the entire issue duration
  3. Multiple labels with different timeframes - tests multiple labels with 21-day spans and a 60-day total issue duration

These tests cover the scenarios you requested: labels applied at creation time and different time spans (7, 21, 30, and 60 days vs the previous 5, 8, 10 day spans). All tests pass.

Changes committed in fc9d45a.

Copilot finished work on behalf of zkoppert August 15, 2025 23:19
@zkoppert
Copy link
Member

@copilot can you fix the failing github actions. I think you need to run the black linter.

Co-authored-by: zkoppert <6935431+zkoppert@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Aug 15, 2025

@copilot can you fix the failing github actions. I think you need to run the black linter.

Fixed the failing GitHub Actions by running black linter on the code. The issue was a long function name that needed to be split across multiple lines according to black's formatting standards. All tests continue to pass (10/10). Commit bb65a49.

Copilot finished work on behalf of zkoppert August 15, 2025 23:51
@zkoppert zkoppert marked this pull request as ready for review August 16, 2025 00:03
@zkoppert zkoppert requested a review from a team as a code owner August 16, 2025 00:03
@Copilot Copilot AI review requested due to automatic review settings August 16, 2025 00:03
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the get_label_metrics function where time spent in labels was being overcounted for closed issues when those labels were removed before the issue was closed. The fix ensures that only labels still active at closure contribute to the final timespan calculation.

  • Added guard condition to check if labels were still applied when issues closed
  • Fixed typo in comment from 'unlabled' to 'unlabeled'
  • Added comprehensive test coverage for various label application/removal scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
labels.py Added guard condition to prevent overcounting metrics for removed labels and fixed spelling typo
test_labels.py Added five new test cases covering different label timing scenarios for closed issues

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@@ -85,11 +85,14 @@ def get_label_metrics(issue: github3.issues.Issue, labels: List[str]) -> dict:
if label in labeled:
# if the issue is closed, add the time from the issue creation to the closed_at time
if issue.state == "closed":
# Only add the final (closed_at - created_at) span if the label was still applied at closure.
if label_last_event_type.get(label) != "labeled":
Copy link
Preview

Copilot AI Aug 16, 2025

Choose a reason for hiding this comment

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

The condition logic is inverted. This should check if the last event was 'unlabeled' to skip the label, not if it wasn't 'labeled'. The condition should be if label_last_event_type.get(label) == "unlabeled": to match the logic in the open issue branch below.

Suggested change
if label_last_event_type.get(label) != "labeled":
if label_last_event_type.get(label) == "unlabeled":

Copilot uses AI. Check for mistakes.

Copy link

@mlabeeb03 mlabeeb03 left a comment

Choose a reason for hiding this comment

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

Fixes my issue, thanks.

Copy link
Member

@zkoppert zkoppert left a comment

Choose a reason for hiding this comment

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

🚀

@jmeridth jmeridth merged commit 9f2ad7e into main Aug 18, 2025
32 checks passed
@jmeridth jmeridth deleted the copilot/fix-c73f4ba5-18ec-4859-91ff-5981aab025bf branch August 18, 2025 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants