Skip to content

Remove report_id from measurement submission #1195

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.

Sign up for GitHub

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

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

Remove report_id from measurement submission #1195

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e38bc1a
remove report id from legacy measurement sumission function
LDiazN Apr 28, 2026
393d254
Remove report_id from anonymous credentials submit
LDiazN Apr 28, 2026
c804c84
Disaggregate bad measurements count by type of error
LDiazN Apr 29, 2026
77a5ecf
disaggregate type of error in anonc submission path
LDiazN Apr 30, 2026
dff6de5
Refactor metadata checking in submission
LDiazN Apr 30, 2026
69a1628
Remove open report from anonc tests
LDiazN Apr 30, 2026
dc5ee4e
simplify typing
LDiazN Apr 30, 2026
198bb1e
Improve docstring
LDiazN Apr 30, 2026
5374c5c
remove unused function
LDiazN Apr 30, 2026
90d52f3
remove unused imports
LDiazN Apr 30, 2026
eea7d17
Set report_id in measurement submission path and add test for checkin…
LDiazN May 7, 2026
a973560
Add check to verify that submit_measurement always sets the report_id
LDiazN May 7, 2026
0582837
Add report_id to the content key instead of root of json
LDiazN May 8, 2026
2e9739d
Refactor validation into check function
LDiazN May 14, 2026
1843154
Add report_id to submission result
LDiazN May 14, 2026
22bc4f4
Move reformating to function constructing msm uid
LDiazN May 14, 2026
b383f67
Restore report_id parsing; add consistency check between report_id an…
LDiazN May 14, 2026
45e46d9
Update tests
LDiazN May 14, 2026
acbf2ba
Add tests to check metadata consistency against report_id
LDiazN May 14, 2026
72c2b81
Remove outdated comment
LDiazN May 14, 2026
41d6bff
Remove uneccesary identation
LDiazN May 14, 2026
8d7dba6
Improve test name and docs
LDiazN May 14, 2026
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
114 changes: 51 additions & 63 deletions ooniapi/services/ooniprobe/src/ooniprobe/routers/reports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,20 @@
from pydantic import Field
from starlette.concurrency import run_in_threadpool

from ..common.config import Settings
from ..common.dependencies import ClickhouseDep
from ..common.metrics import timer
from ..common.routers import BaseModel
from ..common.utils import setnocacheresponse
from ..dependencies import ASNCCReaderDep, SettingsDep
from ..metrics import Metrics
from ..utils import (
MeasurementMetadata,
check_measurement_meta,
error,
generate_report_id,
get_cc_asn,
metadata_from_measurement_content,
normalize_asn,
register_geoip_anomaly,
)
Expand Down Expand Up @@ -107,41 +111,14 @@ async def receive_measurement(
content_encoding: str = Header(default=None),
) -> ReceiveMeasurementResponse | Dict[str, Any]:
"""
Submit measurement
Submit measurement.

The `report_id` path parameter is accepted for URL backwards
compatibility but is ignored: metadata used to identify
and validate the measurement is read from the body
"""
setnocacheresponse(response)
empty_measurement = {}
try:
rid_timestamp, test_name, cc, asn, format_cid, rand = report_id.split("_")
except Exception as e:
log.info(
f"Unexpected report_id {report_id[:200]}. Error: {e}",
)
Metrics.BAD_MEASUREMENTS_CNT.labels(reason="bad_report_id").inc()
raise error("Incorrect format")

# TODO validate the timestamp?
good = len(cc) == 2 and test_name.isalnum() and 1 < len(test_name) < 30
if not good:
log.info("Unexpected report_id %r", report_id[:200])
error("Incorrect format")

try:
asn_i = int(asn)
except ValueError as e:
log.info(f"ASN value not parsable {asn}. Error: {e}")
Metrics.BAD_MEASUREMENTS_CNT.labels(reason="bad_asn").inc()
error("Incorrect format")

if asn_i == 0:
log.info("Discarding ASN == 0")
Metrics.BAD_MEASUREMENTS_CNT.labels(reason="asn_0").inc()
return empty_measurement

if cc.upper() == "ZZ":
log.info("Discarding CC == ZZ")
Metrics.BAD_MEASUREMENTS_CNT.labels(reason="cc_zz").inc()
return empty_measurement

with Metrics.READ_BODY_TIMING.time():
data = await request.body()
Expand All @@ -157,13 +134,21 @@ async def receive_measurement(
error("Incorrect format")

try:
data = await run_in_threadpool(_set_unverified_flag, data)
data, metadata = await run_in_threadpool(
_process_measurement_body, data, settings
)
except Exception as e:
log.info("Failed to parse and modify measurement body")
log.exception(e)
Metrics.BAD_MEASUREMENTS_CNT.labels(reason="bad_json").inc()
error("Incorrect format")

test_name = metadata.test_name
cc = metadata.probe_cc
asn = metadata.probe_asn

check_measurement_meta(test_name, cc, asn)

# Write the whole body of the measurement in a directory based on a 1-hour
# time window
now = datetime.now(timezone.utc)
Expand Down Expand Up @@ -214,7 +199,7 @@ async def receive_measurement(
cc,
asn,
msmt_uid,
data,
metadata,
)
except Exception as e:
log.error(f"Error checking for geoip anomalies: {e}")
Expand Down Expand Up @@ -242,13 +227,37 @@ async def receive_measurement(
Metrics.SEND_S3_CNT.labels(status="fail").inc()
return empty_measurement

def _process_measurement_body(
data: bytes, settings: Settings
) -> Tuple[bytes, MeasurementMetadata]:
"""
- Parse the measurement body
- extract some metadata fields
- set `is_verified="u"`
- set report_id
- re-serialize.
"""

def _set_unverified_flag(data: bytes) -> bytes:
with Metrics.DESERIALIZE_BODY_TIMING.time():
measurement = ujson.loads(data)
measurement["is_verified"] = "u"
json = ujson.loads(data)

assert isinstance(json, dict)

content = json.get("content")
assert isinstance(content, dict)

metadata = metadata_from_measurement_content(content)

json["is_verified"] = "u"
json["content"]["report_id"] = generate_report_id(
Copy link
Copy Markdown
Member

@hellais hellais May 12, 2026

Choose a reason for hiding this comment

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

I don't think we can do this, because it will otherwise result in an inconsistency between what is inside of the measurement already (that the user got through other means) and what we are replacing it with.

In this code path, I think we should be using the report_id that's taken from the URL.

Copy link
Copy Markdown
Contributor Author

@LDiazN LDiazN May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If we do this, we would still have to require the client to make a call to the generate report id function (or change it to generate the report_id on its own?). Which is fine, but we would only remove the report_id requirement from the anonc endpoint and not from the legacy one. Is this what we want?

Copy link
Copy Markdown
Member

@hellais hellais May 13, 2026

Choose a reason for hiding this comment

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

The /report/{report_id} API call already assumes this as the report_id is encoded inside of the URL path and it cannot work without it: https://github.com/ooni/backend/pull/1195/changes#diff-fd68970e397912b47cad3b7957ef7c811249f632e065d349a0161f06fa516591R103.

I don't think we can get rid of requiring the report_id in a way that's backward compatible.

metadata.test_name,
settings,
metadata.probe_cc,
normalize_asn(metadata.probe_asn)
)

with Metrics.SERIALIZE_BODY_TIMING.time():
return ujson.dumps(measurement).encode("utf-8")
return ujson.dumps(json).encode("utf-8"), metadata


def _check_and_register_geoip_anomaly(
Expand All @@ -258,46 +267,25 @@ def _check_and_register_geoip_anomaly(
cc: str,
asn: str,
msmt_uid: str,
data: bytes,
metadata: MeasurementMetadata,
) -> None:
actual_cc, actual_asn = get_cc_asn(request, asn_cc_reader)
if actual_cc != cc or normalize_asn(actual_asn) != normalize_asn(asn):
# expensive: parses measurement body and sends anomaly to clickhouse
platform, software_name, software_version = _parse_metadata(data)
register_geoip_anomaly(
cc,
actual_cc,
asn,
actual_asn,
clickhouse,
msmt_uid,
platform,
software_name,
software_version,
metadata.platform,
metadata.software_name,
metadata.software_version,
)
else:
Metrics.PROBE_CC_ASN_MATCH.inc()


def _parse_metadata(data: bytes) -> Tuple[str, str, str]:
"""
Parse measurement body, and return the following metadata:

platform, software_name, software_version
"""
try:
body = ujson.loads(data.decode("utf-8"))
except Exception as e:
log.error(f"Couldn't parse json body: {e}")
return ("", "", "")
content = body.get("content", {})
annotations = content.get("annotations", {})
platform = annotations.get("platform", "")
software_name = content.get("software_name", "")
software_version = content.get("software_version", "")
return (platform, software_name, software_version)


@timer(name="close_report")
@router.post("/report/{report_id}/close", tags=["reports"])
def close_report(report_id):
Expand Down
69 changes: 20 additions & 49 deletions ooniapi/services/ooniprobe/src/ooniprobe/routers/v1/probe_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,12 @@
PsiphonConfigDep
)
from ...utils import (
check_measurement_meta,
extract_probe_ipaddr,
generate_report_id,
geolookup_probe,
get_cc_asn,
metadata_from_measurement_content,
normalize_asn,
register_geoip_anomaly,
)
Expand Down Expand Up @@ -872,9 +874,8 @@ class SubmitMeasurementResponse(BaseModel):
)


@router.post("/submit_measurement/{report_id}", tags=["anonymous_credentials"])
@router.post("/submit_measurement", tags=["anonymous_credentials"])
async def submit_measurement(
report_id: str,
request: Request,
submit_request: SubmitMeasurementRequest,
response: Response,
Expand All @@ -900,61 +901,29 @@ async def submit_measurement(
- status code 4xx or 5xx: the measurement was not processed nor stored
"""
setnocacheresponse(response)
try:
rid_timestamp, test_name, cc, asn, format_cid, rand = report_id.split("_")
except Exception:
err_msg = f"Incorrect format: unexpected report_id {report_id[:200]}"
log.info(err_msg)
raise HTTPException(
status_code=400,
detail={"error": "incorrect_format", "message": err_msg},
)

# TODO validate the timestamp?
good = len(cc) == 2 and test_name.isalnum() and 1 < len(test_name) < 30
if not good:
err_msg = f"Incorrect format: unexpected report_id {report_id[:200]}"
log.info(err_msg)
raise HTTPException(
status_code=400,
detail={"error": "incorrect_format", "message": err_msg},
)
metadata = metadata_from_measurement_content(submit_request.content)

try:
asn_i = int(asn)
except ValueError:
err_msg = f"Incorrect format: ASN value not parsable {asn}"
log.info(err_msg)
raise HTTPException(
status_code=400,
detail={"error": "incorrect_format", "message": err_msg},
)
test_name = metadata.test_name
cc = metadata.probe_cc
asn = metadata.probe_asn

if asn_i == 0:
log.info("Discarding ASN == 0")
Metrics.MSMNT_DISCARD_ASN0.inc()
raise HTTPException(400, detail = {"error" : "asn_0", "message" : "Measurement discarded, ASN == 0"})
check_measurement_meta(test_name, cc, asn)

if cc.upper() == "ZZ":
log.info("Discarding CC == ZZ")
Metrics.MSMNT_DISCARD_CC_ZZ.inc()
raise HTTPException(400, detail = {"error" : "cc_zz", "message" : "Measurement discarded, CC == ZZ"})
# generate the new report_id
rid = generate_report_id(test_name, settings, cc, normalize_asn(asn))

# Anonymous credentials verification
verification_status, submit_error, submit_response = _verify_submit(
submit_request, manifest, settings
)

annotations = submit_request.content.get("annotations", {})
platform = annotations.get("platform") or ""
software_name = submit_request.content.get("software_name") or ""
software_version = submit_request.content.get("software_version") or ""

data = submit_request.model_dump()

# Add verification-related data.
# use one-letter code for DB, human readable for clients
data["is_verified"] = verification_status.code
data["content"]["report_id"] = rid
data_buff = io.BytesIO()
stream = io.TextIOWrapper(data_buff, "utf-8")
ujson.dump(data, stream)
Expand Down Expand Up @@ -1013,9 +982,9 @@ async def submit_measurement(
cc,
asn,
msmt_uid,
platform,
software_name,
software_version,
metadata.platform,
metadata.software_name,
metadata.software_version,
)
except Exception as e:
log.error(f"Error checking for geoip anomalies: {e}")
Expand All @@ -1031,19 +1000,21 @@ async def submit_measurement(
Metrics.SEND_FASTPATH_CNT.labels(status="fail").inc()

# wasn't possible to send msmnt to fastpath, try to send it to s3
data_buff.seek(0)
ts_prefix = now.strftime("%Y%m%d%H")
tn = test_name.replace("_", "")
s3_key = f"postcans/{ts_prefix}/{ts_prefix}_{cc}_{tn}/{msmt_uid}.post"
try:
await run_in_threadpool(
request.app.state.s3_client.upload_fileobj,
data_buff,
io.BytesIO(data_bin),
Bucket=settings.failed_reports_bucket,
Key=report_id,
Key=s3_key,
)
except Exception as exc:
log.error(f"Unable to upload measurement to s3. Error: {exc}")
Metrics.SEND_S3_FAILURE.inc()

log.error(f"Unable to send report to fastpath. report_id: {report_id}")
log.error(f"Unable to send report to fastpath. measurement_uid: {msmt_uid}")
Metrics.MISSED_MSMNTS.inc()
return SubmitMeasurementResponse(
Copy link
Copy Markdown
Member

@hellais hellais May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to return to the caller the report_id that was generated as well? I suppose it doesn't hurt since it's a value which we generated in here and the client has no other way of knowing it.

Copy link
Copy Markdown
Contributor Author

@LDiazN LDiazN May 13, 2026

Choose a reason for hiding this comment

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

I think the report_id is redundant to the measurement_uid in this context (it's unique per measurement). The best value we could get out of it is restoring it in the future, but in that case the probe would probably already know it before sending the measurement

But I agree it wouldn't hurt to have add it

Copy link
Copy Markdown
Member

@hellais hellais May 13, 2026

Choose a reason for hiding this comment

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

Yes I agree that it's redundant. We should check though with @aanorbel and @sdsantos if this is though still required by the mobile apps for something (I seem to recall this was still being stored in the in-app DB).

Copy link
Copy Markdown
Contributor Author

@LDiazN LDiazN May 14, 2026

Choose a reason for hiding this comment

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

Norbel suggested that we keep the report_id in the response model, so I added that 👍

measurement_uid=msmt_uid,
Expand Down
Loading
Loading