Skip to content

Remove report_id from measurement submission#1195

Open
LDiazN wants to merge 22 commits into
masterfrom
remove-report-id
Open

Remove report_id from measurement submission#1195
LDiazN wants to merge 22 commits into
masterfrom
remove-report-id

Conversation

@LDiazN
Copy link
Copy Markdown
Contributor

@LDiazN LDiazN commented Apr 28, 2026

This PR will remove the requirement of a report id from measurement submission

  1. The /receive_measurement endpoint will still expect the report_id parameter, but this will no longer be used. Instead, a pseudo-report_id will be constructed using metadata from the measurement body itself
  2. The /submit_measurement (the new anonymous credentials version of measurement submission) will remove it entirely from the path

Note that the report_id will no longer be usable to group measurements that were taken in the same session, as every measurement will have a distinct report_id

After this PR, the report_id will be considered a legacy key, only kept for backwards compatibility

A call to POST /report to get the report_id will no longer be required before submitting measurements.

closes #1139

@LDiazN LDiazN self-assigned this Apr 28, 2026
@LDiazN LDiazN marked this pull request as ready for review April 30, 2026 11:09
@LDiazN LDiazN requested a review from hellais April 30, 2026 11:09
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 77.09497% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.70%. Comparing base (dbf3cca) to head (8d7dba6).
⚠️ Report is 28 commits behind head on master.

Files with missing lines Patch % Lines
ooniapi/services/ooniprobe/src/ooniprobe/utils.py 41.66% 30 Missing and 5 partials ⚠️
...niprobe/src/ooniprobe/routers/v1/probe_services.py 73.33% 4 Missing ⚠️
ooniapi/services/ooniprobe/tests/test_anoncred.py 94.87% 2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (77.09%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project check has failed because the head coverage (92.70%) is below the target coverage (95.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
+ Coverage   91.84%   92.70%   +0.86%     
==========================================
  Files          85       78       -7     
  Lines        7907     7388     -519     
  Branches      476      439      -37     
==========================================
- Hits         7262     6849     -413     
+ Misses        539      448      -91     
+ Partials      106       91      -15     
Flag Coverage Δ
ooniauth 100.00% <ø> (ø)
oonifindings 97.32% <ø> (+<0.01%) ⬆️
oonimeasurements 88.74% <ø> (+<0.01%) ⬆️
ooniprobe 90.34% <77.09%> (+1.83%) ⬆️
oonirun 98.85% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

"""
test_name = str(content.get("test_name") or "").lower().replace("_", "")

cc = str(content.get("probe_cc") or "").upper().replace("_", "")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that since this is inside of the actual measurement content, we want to be a bit more strict about what we accept. That is to say we should enforce that the probe_cc is meeting all this criteria and not return a default value if it's inconsistent.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually in looking at this further, I think the validation should live in the check function and in here you should just return the raw values as they as to perform validation later.


raw_asn = str(content.get("probe_asn") or "AS0").upper()
if len(raw_asn) > 12 or len(raw_asn) < 3 or not raw_asn.startswith("AS"):
raw_asn = "AS0"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment as above, the client should be setting a good value here, if they aren't we should be providing an error.

Checks metadata consistency, raising an HTTPException and stopping
ingestion when an inconsistency is detected
"""
asn_i = normalize_asn(probe_asn)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure it's correct to be doing normalization here, since the value is coming from the measurement body. If it's not in a format compatible with the spec, we should return an error to client.

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

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

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

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

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 👍

metadata = metadata_from_measurement_content(content)

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

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

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

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.

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.

Drop report_id requirement for submitting measurements

2 participants