[mock_uss] Add user notifications for missing fields in injected flights #911
Conversation
the-glu
force-pushed
the
user-notifications-missing-fields
branch
2 times, most recently
from
f5677c5 to
b6f033c
Compare
the-glu
force-pushed
the
user-notifications-missing-fields
branch
from
1988c8f to
eecc6f4
Compare
| @@ -29,3 +31,20 @@ def record_notification( | |||
| self.user_notifications.append( | |||
| UserNotification(observed_at=observed_at_time, message=message) | |||
| ) | |||
|
|
|||
| def create_notifications_if_nedded(self, record: "database.TestRecord"): | |||
There was a problem hiding this comment.
This is going to trigger a notification for every single missing field in any telemetry point. That's not realistic since we'd want that only if there are missing fields for a while, but that could be acceptable behavior for the mock USS.
However what it is not covering is the lack of any telemetry point for a while. Which we will need for NET0400. Could you add that? Thanks!
There was a problem hiding this comment.
Which we will need for NET0400. Could you add that? Thanks!
We planned with @Shastick to implement that one in a different PR, this is only for NET0030 on purpose :)
This is going to trigger a notification for every single missing field in any telemetry point. That's not realistic since we'd want that only if there are missing fields for a while, but that could be acceptable behavior for the mock USS.
For the mock uss we probably want that for testing: It's reporting all missing fields with location on purpose if debugging is needed (and that shouldn't add much overhead, when testing we do only set one missing field in one telemetry). Also I'm not sure, but complaining immediately is not an issue based on the standard no?
There was a problem hiding this comment.
Yep, I don't see a reason to limit ourselves to a single notification, and the current approach makes the logic very simple.
The intention for the scenario is to assume that we have at least one notification sent (ie, the notification count over the period is non-zero, and/or the total number of notifications at the end of the scenario is higher than when it started.
There was a problem hiding this comment.
We planned with @Shastick to implement that one in a different PR, this is only for NET0030 on purpose :)
OK!
Also I'm not sure, but complaining immediately is not an issue based on the standard no?
Not an issue indeed regarding standard indeed.
The intention for the scenario is to assume that we have at least one notification sent (ie, the notification count over the period is non-zero, and/or the total number of notifications at the end of the scenario is higher than when it started.
Alright 👍
the-glu
force-pushed
the
user-notifications-missing-fields
branch
2 times, most recently
from
d3966b8 to
6fcca5a
Compare
|
|
||
| filtered_telemetry.append(telemetry) | ||
|
|
||
| self.telemetry = filtered_telemetry |
There was a problem hiding this comment.
This is changing behavior of TestFlight.telemetry: at init this does not have anymore the invalid telemetry. Why is that needed? At a quick glance this is notably used by the uss_qualifier injection logic.
In all cases I think that we wouldn't expect the constructor to modify existing data of injection.TestFlight. Adding some fields/accessors though yes. So if we need to have some additionally processed telemetry, rather add it on top of existing data?
There was a problem hiding this comment.
It's needed because TestFlight functions / helpers are not supporting invalid telemetry: there are a lot of fields accessed in various helpers everywhere (like get_span just bellow ^^').
Maybe we can add a flag to do the telemetry filtering only on purpose, when we inject know-bad telemetry data and have the tests crash with bad telemetry not-on-purpose? But if I'm remember correctly it was not easy to add, as this class is initialized 'deep bellow' under the test layer.
There was a problem hiding this comment.
(I forgot: the flag is already added in #916, but inverted (the default is to filter))
There was a problem hiding this comment.
So after discussions, I back ported the flag form #916 :)
the-glu
force-pushed
the
user-notifications-missing-fields
branch
from
6fcca5a to
9febd30
Compare
the-glu
force-pushed
the
user-notifications-missing-fields
branch
from
9febd30 to
fd6f219
Compare
| # We filter out bad telemetry but keep a copy in raw_telemetry | ||
| self.raw_telemetry = self.telemetry | ||
|
|
||
| verify_telemetry_data = kwargs.pop("verify_telemetry_data", True) |
There was a problem hiding this comment.
Any reason why not putting this as parameter of init with a default value?
(should be OK for existing callers if it is a default value, or not?)
There was a problem hiding this comment.
Also I'd clarify the name with e.g. filter_invalid_telemetry
I'd expect a parameter named validate to return an error if invalid.
There was a problem hiding this comment.
If you have callers using args and not kwargs, it will behave differently, because it will change the arguments order, so this ensure backward compatibility :)
There was a problem hiding this comment.
(Field has been renamed)
the-glu
force-pushed
the
user-notifications-missing-fields
branch
from
fd6f219 to
aecd341
Compare
Follow #910
Tested, I added a filter + a new raw_telemetry field for internal purpose, because everything was crashing with bad telemetry data ^^'