Repository navigation
BB2-4572/4911/5193: ITS-log initial integration - #1746
JamesDemeryNava wants to merge 65 commits into
Conversation
…evel ones, calculate in log_global_state_metrics
| parser.add_argument( | ||
| '-l', | ||
| '--post-to-its-log', | ||
| action='store_false', |
There was a problem hiding this comment.
I think this is backwards, passing the argument should store_true
There was a problem hiding this comment.
If the argument is not passed, we want it to evaluate as false. From reading the docs and running the log_global_state_metrics command that has worked, so if the argument is passed, it evaluates as true.
| group_timestamp = format_timestamp(datetime.now()) | ||
|
|
||
| report_flag = False if options.get('no_report', None) else True | ||
| its_log_flag = False if options.get('post_to_its_log', None) else True |
There was a problem hiding this comment.
Not sure about this
There was a problem hiding this comment.
Admittedly, this is confusing.
If we set the flag to False, then options.get() will return False. This will then cause the expression to evaluate to True.
If we set the flag to True, then options.get() will return True. This will cause the expression to evaluate to False.
This statement is, I think, inverting the value of the flag passed in.
There was a problem hiding this comment.
From the testing i've done, if the command has the --post-to-its-log flag, we will ping its-log. If the command is run without the --post-to-its-log flag, we don't post to its-log 🤷 I can try to refactor to make this line less confusing if needed.
| return | ||
| payload = self._build_payload(updated_log_message) | ||
|
|
||
| threading.Thread(target=self._post_to_api, args=(payload,), daemon=True).start() |
There was a problem hiding this comment.
do we want to create a new thread for each invocation?
There was a problem hiding this comment.
Reading more on this, but I don't think so. There should be some kind of cap. Maybe we limit it to 10 workers initially and see how that performs.
annamontare-nava
left a comment
annamontare-nava
left a comment
There was a problem hiding this comment.
A few comments to address, and need to get validation working on my end, but otherwise good!
| try: | ||
| result = requests.post( | ||
| 'http://host.docker.internal:8888/v1/log/create', | ||
| headers={'x-api-key': '1234567890123456123456789012345612345678901234561234567890123456'}, |
JIRA Ticket:
BB2-4572
BB2-4911
BB2-5193
What Does This PR Do?
This PR implements pinging a logging API, currently known as its-log, for specific Blue Button log types. This is part of a larger effort to implement a new data pipeline that is simpler and easier to maintain. We add an environment variable, ITS_LOG_API_ENABLED, which controls whether or not we ping the its-log API.
There is a Confluence page here with details on how to run its-log. You will need to reference that page in order to test this PR. Be sure to download DBeaver before reviewing this and running its-log.
What Should Reviewers Watch For?
If you're reviewing this PR, please check for these things in particular:
Validation
What Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
security engineer's approval. @sb-DarenDean @sb-benohe checking this because once this is merged and the waffle switch is enabled (which won't happen any time soon, still need to add the terraform code for the new architecture), BB will be pinging a new API to help track metrics data.
Any Migrations?
etc)