-
Notifications
You must be signed in to change notification settings - Fork 20
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] NET0260-a performance check for querying flights in an area from a SP #159
[uss_qualifier] NET0260-a performance check for querying flights in an area from a SP #159
Conversation
Note: not sure if we want to keep the small utility for exploring a report's content? It was helpful to me to learn a bit about how the classes are structured and expect it to help a bit more if we have more complex aggregated checks, but I can also remove it. |
Before a review, could you ensure the CI passes? |
fa91295
to
86d160a
Compare
Looks like the failure was unrelated. Rebased and PR still green and ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check does not seem to validate any query:
2023-08-15 12:43:57.628 | INFO | monitoring.uss_qualifier.scenarios.scenario:record_note:263 - Note: uss1/flights -> skipped check: no relevant queries
2023-08-15 12:43:57.629 | INFO | monitoring.uss_qualifier.scenarios.scenario:record_note:263 - Note: uss2/flights -> skipped check: no relevant queries
I assume that is not expected?
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check does not seem to validate any query
@mickmis was this for v22a or v19? IIRC I had this passing locally when tested against v22a only.
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
d729a19
to
6966e85
Compare
59dce4d
to
484bd82
Compare
47d942f
to
4ba121d
Compare
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
monitoring/uss_qualifier/configurations/dev/library/environment.yaml
Outdated
Show resolved
Hide resolved
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
self.end_test_scenario() | ||
|
||
def _sp_flights_area_times_step(self): | ||
pattern = re.compile(r"/flights\?view=") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize now an issue with the regex (which is also present in the _dp_display_data_times_step I wrote), which is that it is valid to have another parameter after the ?
and before the view
parameter. The regex should be updated accordingly to reflect this. (and if you don't mind fixing the other one at the same time that would be great)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A side question here: does it matter if there are additional parameters being passed? Note that for the queries I am interested in, there actually are more params: Ie http://host.docker.internal:8081/mock/ridsp/v2/uss/flights?view=37.17008162687164%2C-80.60282480412947%2C37.22330680061591%2C-80.5495996303852&recent_positions_duration=60
, and there's no guarantee about their order to regex matching too precisely could cause troubles as well.
What might make sense is to give the full method path to avoid accidentally matching something else, but the goal being to find queries to a particular endpoint, I'm not sure there is a need to validate the parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A side question here: does it matter if there are additional parameters being passed? Note that for the queries I am interested in, there actually are more params: Ie http://host.docker.internal:8081/mock/ridsp/v2/uss/flights?view=37.17008162687164%2C-80.60282480412947%2C37.22330680061591%2C-80.5495996303852&recent_positions_duration=60, and there's no guarantee about their order to regex matching too precisely could cause troubles as well.
For the requirement NET0260, no it does not matter.
What might make sense is to give the full method path to avoid accidentally matching something else, but the goal being to find queries to a particular endpoint, I'm not sure there is a need to validate the parameters?
Indeed you are right, no need to validate the parameters here for NET0260.
6c3a882
to
db9a0e8
Compare
monitoring/uss_qualifier/configurations/dev/library/environment.yaml
Outdated
Show resolved
Hide resolved
monitoring/uss_qualifier/configurations/dev/library/environment.yaml
Outdated
Show resolved
Hide resolved
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
Additionally, looking at the CI logs something looks wrong. I think it's because of the uss_base_urls having only the hostname and not the rest of the path. For v22a there is only 1 participant and only the /flights call being evaluated:
For v19 nothing is evaluated:
|
db9a0e8
to
535b8be
Compare
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
Indeed, something is off here. Though we match URLS by prefix for mapping them to participants (via edit: on master (see this run) v22a is only doing the aggregate checks for USS1. It's either expected or broken? v22a aggreagate checks on master:
|
8669c7c
to
d7f9362
Compare
Fixed a bug I introduced in We now have, for v19:
and for v22a:
|
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
Thanks for the feedback, @BenjaminPelletier, both suggestions seem doable to me, I'll get back to you if I need any clarification. |
Taking this to a draft state again until it's in a satisfactory state |
…playing flights in a given area
d7f9362
to
41107d6
Compare
41107d6
to
75290c1
Compare
Ready for another pass of review. The current version is working, as far as I can trust the logs:
v19 also has output showing that queries are properly analysed. |
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
monitoring/uss_qualifier/scenarios/astm/netrid/common/aggregate_checks.py
Outdated
Show resolved
Hide resolved
@@ -234,6 +234,9 @@ def evaluate_system_instantaneously( | |||
) | |||
for q in sp_observation.queries: | |||
self._test_scenario.record_query(q) | |||
# Keep track of who the query is being made to | |||
# for later use in aggregate checks | |||
q.server_id = self._dss.participant_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sp_observation
is a FetchedFlights
which consists of a query to the DSS, queries to various USSs for flights, and queries to various USSs for flight details. The DSS participant ID only applies to the first of those, so we wouldn't want to apply it to all the USS queries as well.
At this point, we don't know which of sp_observation.uss_flight_queries
belongs to which (if any) of the test participants -- it's conceivable that another USS not participating in the test may nonetheless be in the interoperability ecosystem, causing uss_qualifier and other USSs to interact with it during the test. We only deduce which interoperability ecosystem SPs must be which test participants here when we map the observed flights to injected flights. Since (at that point) we know which participant each injected flight was sent to, we can, at this point, associate observed flights with test participants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, indeed.
Stamping the query with the id now happens after the mapping was established.
75290c1
to
4cf5e85
Compare
4cf5e85
to
145d461
Compare
PR updated, log outputs for the checks are still looking as expected. |
Ensures that the multiple queries to a SP for requesting flights in an area match the required response times.
We have several queries done to that endpoint during the tests already, so this simply extends our aggregate checks.
Observers and service_providers defined in
environment.yaml
(and their respecive Python classes) now have auss_base_urls
array parameters that we use to map queries to USS'es. This is required as (to the best of my understanding) we need to be told which urls will be registered for an SP at the DSS.Note this also includes a little utility for displaying information about a report.