[ft-benchmark] fill actor and context db fields properly #11663
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11663 +/- ##
==========================================
- Coverage 71.63% 71.61% -0.03%
==========================================
Files 787 787
Lines 161118 161104 -14
Branches 161118 161104 -14
==========================================
- Hits 115415 115368 -47
- Misses 40653 40687 +34
+ Partials 5050 5049 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| parser.add_argument('--user', type=str, default='unknown', help='User name') | ||
| parser.add_argument('--context', | ||
| type=str, | ||
| default='unknown', |
There was a problem hiding this comment.
nit: Perhaps make user and context required arguments without default, to make sure these fields have meaningful values.
There was a problem hiding this comment.
Maybe I agree, but maybe some time we will prefer to see row in db with unknown, unknown rather than see nothing. If you strongly confident that we want this arguments be required let me know I will change them
| @@ -111,6 +111,11 @@ def commit_to_db(data: dict) -> None: | |||
| type=int, | |||
| required=True, | |||
| help='Number of users') | |||
| parser.add_argument('--user', type=str, default='unknown', help='User name') | |||
There was a problem hiding this comment.
nit: I think user is a bit ambiguous here. initiator might be clearer and would be in line with the db column name. (In the postgres db naming a column user lead to lint errors, so instead it's named initiator.)
There was a problem hiding this comment.
Yep, it would be better to do this naming consistent with db scheme, but then I will need rename this across all files so probably will do it in another PR
Before this changes
initiatorandcontextdb fields was filled by hardcoded constants. Now they switched to command line arguments. Probably it makes sense to do this arguments required and not default them to "unknown", but don't think it is very important