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

[uss_qualifier] warn upon unattributed or untyped queries, set query types in many instances #436

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Dec 21, 2023

This should fix most if not all non-attributed queries in the context of F3411 and resolve #431

I found no warnings when running configurations.dev.netrid_v19 and configurations.dev.netrid_v22a: there might be a few stray ones remaining with other configurations, and these can be fixed on-the-go when we run into them.

@Shastick Shastick force-pushed the query-details-attribution branch from 5ffd9c3 to 6167a65 Compare December 21, 2023 11:41
Comment on lines 40 to 52
SQUELCH_WARN_ON_QUERY_TYPE = [
# Posting an ISA is done for notifications: we can't always know the participant ID
QueryType.F3411v19USSPostIdentificationServiceArea,
QueryType.F3411v22aUSSPostIdentificationServiceArea,
# When querying for display data and flight details, we don't always know the participant ID
QueryType.F3411v19USSGetDisplayData,
QueryType.F3411v22aUSSGetDisplayData,
QueryType.F3411v19USSGetFlightDetails,
QueryType.F3411v22aUSSGetFlightDetails,
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for this solution to avoid noisy warnings that have no obvious way of being fixed.

For situations where we should be able to set the participant_id, we still have the aggregate checks logging a single warning with all queries missing a participant_id: this feels like an acceptable trade-off to me.

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good approach to me :)

@Shastick Shastick marked this pull request as ready for review December 21, 2023 11:48
@Shastick Shastick force-pushed the query-details-attribution branch from 6167a65 to b4f6ee6 Compare December 22, 2023 14:33
monitoring/monitorlib/fetch/__init__.py Outdated Show resolved Hide resolved
monitoring/monitorlib/fetch/__init__.py Outdated Show resolved Hide resolved

# Flight injection (test harness)

F3411v19USSCreateTest = "rid.f3411.v19.sp.createTest"
Copy link
Member

Choose a reason for hiding this comment

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

We want these IDs to be universally unique, so starting with the organization that defined them is good, then following the specificity chain down -- here, I would suggest interuss.automated_testing.rid.v1.injection.createTest (and similarly for the other following endpoints).

monitoring/monitorlib/fetch/__init__.py Outdated Show resolved Hide resolved
monitoring/monitorlib/fetch/__init__.py Outdated Show resolved Hide resolved
@Shastick Shastick force-pushed the query-details-attribution branch from b4f6ee6 to 02a4bb5 Compare January 8, 2024 08:22
@Shastick
Copy link
Contributor Author

Shastick commented Jan 8, 2024

Did a pass of cleanup and renaming. Happy new year!

@Shastick Shastick marked this pull request as draft January 8, 2024 08:28
@Shastick Shastick force-pushed the query-details-attribution branch 6 times, most recently from 4af7674 to 7b14385 Compare January 8, 2024 09:49
@Shastick Shastick marked this pull request as ready for review January 8, 2024 10:28
Copy link
Member

@BenjaminPelletier BenjaminPelletier left a comment

Choose a reason for hiding this comment

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

Looks good; just one small comment

Comment on lines 537 to 538
is_mutation = isa_version is None
mutation = "create" if is_mutation else "update"
Copy link
Member

Choose a reason for hiding this comment

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

I think there are two errors which net out to the desired behavior that would be fixed by renaming is_mutation to is_creation". (When isa_version` is None, that's a creation rather than a mutation/update)

Suggested change
is_mutation = isa_version is None
mutation = "create" if is_mutation else "update"
is_creation = isa_version is None
mutation = "create" if is_creation else "update"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you! It's pretty rare that two wrongs will do a right :)

@Shastick Shastick force-pushed the query-details-attribution branch from 7b14385 to c7ffe33 Compare January 10, 2024 08:25
@BenjaminPelletier BenjaminPelletier merged commit a1d3402 into interuss:main Jan 10, 2024
9 checks passed
github-actions bot added a commit that referenced this pull request Jan 10, 2024
…types in many instances (#436)

* [uss_qualifier] warn upon unattributed or untyped queries

* fixes a1d3402
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.

Annotate nearly all uss_qualifier queries with participant and type
2 participants