Skip to content
This repository was archived by the owner on Jun 13, 2025. It is now read-only.

Bundle Analysis: fix trends when no data in query range #1267

Merged
JerrySentry merged 5 commits into from
Apr 3, 2025
Merged

Bundle Analysis: fix trends when no data in query range #1267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea68374
Bundle Analysis: fix trends when no data in query range
JerrySentry Apr 2, 2025
42244ae
Merge branch 'main' into ba_trends
JerrySentry Apr 3, 2025
1b25d6d
fix test
JerrySentry Apr 3, 2025
692be10
fix test 2
JerrySentry Apr 3, 2025
6a9370b
update typing
JerrySentry Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions graphql_api/actions/measurements.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def measurements_by_ids(
before: datetime,
after: Optional[datetime] = None,
branch: Optional[str] = None,
) -> Dict[int, List[Dict[str, Any]]]:
Copy link
Copy Markdown
Contributor

@ajay-sentry ajay-sentry Apr 3, 2025

Choose a reason for hiding this comment

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

was this change on purpose? Should we also change the type on 41 as well

Copy link
Copy Markdown
Contributor Author

@JerrySentry JerrySentry Apr 3, 2025

Choose a reason for hiding this comment

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

yeah because when this function was made the measurable_id was only ints representing the flag IDs for flag measurements, but it can be also str for component names or BA reports.

Yes updating line 41.

) -> Dict[Any, List[Dict[str, Any]]]:
queryset = MeasurementSummary.agg_by(interval).filter(
name=measurable_name,
owner_id=repository.author_id,
Expand All @@ -38,7 +38,7 @@ def measurements_by_ids(
)

# group by measurable_id
measurements: Dict[int, List[Dict[str, Any]]] = {}
measurements: Dict[Any, List[Dict[str, Any]]] = {}
for measurement in queryset:
measurable_id = measurement["measurable_id"]
if measurable_id not in measurements:
Expand Down
272 changes: 272 additions & 0 deletions graphql_api/tests/test_bundle_analysis_measurements.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3128,3 +3128,275 @@ def test_bundle_report_measurements_only_unknown(self, get_storage_service):
"name": "super",
},
}

@patch("graphql_api.dataloader.bundle_analysis.get_appropriate_storage_service")
def test_bundle_report_measurements_no_data_in_range(self, get_storage_service):
storage = MemoryStorageService({})
get_storage_service.return_value = storage

with open("./services/tests/samples/bundle_with_uuid.sqlite", "rb") as f:
storage_path = StoragePaths.bundle_report.path(
repo_key=ArchiveService.get_archive_hash(self.repo),
report_key=self.head_commit_report.external_id,
)
storage.write_file(get_bucket_name(), storage_path, f)

query = """
query FetchMeasurements(
$org: String!,
$repo: String!,
$commit: String!
$filters: BundleAnalysisMeasurementsSetFilters
$orderingDirection: OrderingDirection!
$interval: MeasurementInterval!
$before: DateTime!
$after: DateTime!
) {
owner(username: $org) {
repository(name: $repo) {
... on Repository {
commit(id: $commit) {
bundleAnalysis {
bundleAnalysisReport {
__typename
... on BundleAnalysisReport {
bundle(name: "super") {
name
measurements(
filters: $filters
orderingDirection: $orderingDirection
after: $after
interval: $interval
before: $before
){
assetType
name
measurements {
avg
min
max
timestamp
}
}
}
}
}
}
}
}
}
}
}
"""

# Test without using asset type filters
variables = {
"org": self.org.username,
"repo": self.repo.name,
"commit": self.commit.commitid,
"orderingDirection": "ASC",
"interval": "INTERVAL_1_DAY",
"after": "2024-06-08",
"before": "2024-06-09",
"filters": {},
}
data = self.gql_request(query, variables=variables)
commit = data["owner"]["repository"]["commit"]

assert commit["bundleAnalysis"]["bundleAnalysisReport"] == {
"__typename": "BundleAnalysisReport",
"bundle": {
"measurements": [
{
"assetType": "ASSET_SIZE",
"measurements": [
{
"avg": 4126.0,
"max": 4126.0,
"min": 4126.0,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
],
"name": "asset-*.js",
},
{
"assetType": "ASSET_SIZE",
"measurements": [],
"name": "asset-*.js",
},
{
"assetType": "ASSET_SIZE",
"measurements": [],
"name": "asset-*.js",
},
{
"assetType": "FONT_SIZE",
"measurements": [
{
"avg": 50.0,
"max": 50.0,
"min": 50.0,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
],
"name": None,
},
{
"assetType": "IMAGE_SIZE",
"measurements": [
{
"avg": 500.0,
"max": 500.0,
"min": 500.0,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
],
"name": None,
},
{
"assetType": "JAVASCRIPT_SIZE",
"measurements": [
{
"avg": 5708.0,
"max": 5708.0,
"min": 5708.0,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
],
"name": None,
},
{
"assetType": "REPORT_SIZE",
"measurements": [
{
"avg": 6263.0,
"max": 6263.0,
"min": 6263.0,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
],
"name": None,
},
{
"assetType": "STYLESHEET_SIZE",
"measurements": [
{
"avg": 5.0,
"max": 5.0,
"min": 5.0,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
],
"name": None,
},
{
"assetType": "UNKNOWN_SIZE",
"measurements": [
{
"avg": 0.0,
"max": 0.0,
"min": 0.0,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
],
"name": None,
},
],
"name": "super",
},
}

# Test with using asset type filters
variables = {
"org": self.org.username,
"repo": self.repo.name,
"commit": self.commit.commitid,
"orderingDirection": "ASC",
"interval": "INTERVAL_1_DAY",
"after": "2024-06-07",
"before": "2024-06-10",
"filters": {"assetTypes": "JAVASCRIPT_SIZE"},
}
data = self.gql_request(query, variables=variables)
commit = data["owner"]["repository"]["commit"]

assert commit["bundleAnalysis"]["bundleAnalysisReport"] == {
"__typename": "BundleAnalysisReport",
"bundle": {
"measurements": [
{
"assetType": "JAVASCRIPT_SIZE",
"measurements": [
{
"avg": 5708.0,
"max": 5708.0,
"min": 5708.0,
"timestamp": "2024-06-07T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-08T00:00:00+00:00",
},
{
"avg": None,
"max": None,
"min": None,
"timestamp": "2024-06-09T00:00:00+00:00",
},
{
"avg": 26708.0,
"max": 26708.0,
"min": 26708.0,
"timestamp": "2024-06-10T00:00:00+00:00",
},
],
"name": None,
},
],
"name": "super",
},
}
11 changes: 3 additions & 8 deletions graphql_api/tests/test_pull.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from datetime import datetime, timedelta
from unittest.mock import patch

import pytest
from django.test import TestCase
from freezegun import freeze_time
from shared.api_archive.archive import ArchiveService
Expand Down Expand Up @@ -460,25 +459,21 @@ def test_with_complete_pull_request(self):
"behindByCommit": "1089nf898as-jdf09hahs09fgh",
}

@pytest.mark.skip(
reason="Skipping due to https://github.com/codecov/engineering-team/issues/3358"
)
def test_compare_bundle_analysis_missing_reports(self):
repository = RepositoryFactory(author=self.owner)
head = CommitFactory(
repository=repository,
repository=self.repository,
author=self.owner,
commitid="cool-commit-id",
totals={"c": "78.38", "diff": [0, 0, 0, 0, 0, "14"]},
)
compared_to = CommitFactory(
repository=repository,
repository=self.repository,
author=self.owner,
commitid="blah",
)

my_pull = PullFactory(
repository=repository,
repository=self.repository,
author=self.owner,
head=head.commitid,
compared_to=compared_to.commitid,
Expand Down
14 changes: 9 additions & 5 deletions services/bundle_analysis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ def __init__(
@sentry_sdk.trace
def _compute_measurements(
self, measurable_name: str, measurable_ids: List[str]
) -> Dict[int, List[Dict[str, Any]]]:
) -> Dict[str, List[Dict[str, Any]]]:
all_measurements = measurements_by_ids(
repository=self.repository,
measurable_name=measurable_name,
Expand All @@ -463,11 +463,13 @@ def _compute_measurements(
after=self.after,
before=self.before,
branch=self.branch,
)
) or {measurable_ids[0]: []}

# Carry over previous available value for start date if its value is null
for measurable_id, measurements in all_measurements.items():
if self.after is not None and measurements[0]["timestamp_bin"] > self.after:
if self.after is not None and (
not measurements or measurements[0]["timestamp_bin"] > self.after
):
carryover_measurement = measurements_last_uploaded_before_start_date(
owner_id=self.repository.author.ownerid,
repo_id=self.repository.repoid,
Expand All @@ -481,8 +483,10 @@ def _compute_measurements(
# If there isn't any measurements before the start date range, measurements will be untouched
if carryover_measurement:
value = Decimal(carryover_measurement[0]["value"])
carryover = dict(measurements[0])
carryover["timestamp_bin"] = self.after
carryover = dict(measurements[0]) if measurements else {}
carryover["timestamp_bin"] = self.after.replace(
Copy link
Copy Markdown
Contributor

@ajay-sentry ajay-sentry Apr 3, 2025

Choose a reason for hiding this comment

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

is this to "zero out" the timestamp?

Copy link
Copy Markdown
Contributor Author

@JerrySentry JerrySentry Apr 3, 2025

Choose a reason for hiding this comment

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

yes, in the downstream when building the list of measurements to return it needs to have 00:00:00.000 as the time, that is expected because normally we pull the data from the aggregate tables, naturally it always agg by n days and so the time is always 0s. But in this case when we carry over the last datapoint we don't query from the aggregate tables, instead it comes from the un-agg'd table so the time is when it got inserted. So this part would zero out the times to comply with the logic later.

Copy link
Copy Markdown
Contributor

@ajay-sentry ajay-sentry Apr 3, 2025

Choose a reason for hiding this comment

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

ahhhh okay makes sense

hour=0, minute=0, second=0, microsecond=0
)
carryover["min"] = value
carryover["max"] = value
carryover["avg"] = value
Expand Down
Loading
Loading