Project

General

Profile

Actions

Idea #13147

closed

[arv-put] Add request ID to requests for tracability

Added by Tom Morris about 6 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Story points:
1.0
Release relationship:
Auto

Description

Enhancing the Python SDK as needed to support the feature


Subtasks 1 (0 open1 closed)

Task #13156: Review 13147-arvput-request-idResolvedPeter Amstutz03/06/2018Actions

Related issues

Related to Arvados - Feature #12167: [SDKs] [API] [keepstore] [keepproxy] Facilitate request tracing for all servicesResolvedTom Clegg08/24/2017Actions
Actions #1

Updated by Tom Morris about 6 years ago

  • Related to Feature #12167: [SDKs] [API] [keepstore] [keepproxy] Facilitate request tracing for all services added
Actions #2

Updated by Lucas Di Pentima about 6 years ago

  • Status changed from New to In Progress
Actions #3

Updated by Lucas Di Pentima about 6 years ago

Updates at 96f657210 - branch 13147-arvput-request-id
Test run: https://ci.curoverse.com/job/developer-run-tests/632/

Created a custom log formatter that appends the request-id on error messages. This formatter is used to replace the arvados formatter when running arv-put.

Actions #4

Updated by Peter Amstutz about 6 years ago

Wouldn't the X-Request-Id check always pass, because it always prints out the request id at the beginning? Maybe the test run should include "--silent"?

    def test_api_error_handling(self):
        coll_save_mock = mock.Mock(name='arv.collection.Collection().save_new()')
        coll_save_mock.side_effect = arvados.errors.ApiError(
            fake_httplib2_response(403), b'{}')
        with mock.patch('arvados.collection.Collection.save_new',
                        new=coll_save_mock):
            with self.assertRaises(SystemExit) as exc_test:
                self.call_main_with_args(['--silent', '/dev/null'])
            self.assertLess(0, exc_test.exception.args[0])
            self.assertLess(0, coll_save_mock.call_count)
            self.assertEqual("", self.main_stdout.getvalue())
            # Mock request id is added on log formatter at setUp
            self.assertRegex(
                self.main_stderr.getvalue(), r'\(X-Request-Id: req-testing123\)\n')

On further inspection, I guess that's not strictly true, because the regex is testing for "(X-Request-Id: req-testing123)" with the parenthesis as part of the match, which only shows up as part of an error message.

Another thought, if this adds the same request_id to every log message at ERROR or DEBUG level, that seems redundant. Maybe just add it once the first time?

Separately, I wonder if always logging X-Request-Id at INFO level at the beginning is desirable, even when the transfer was a success, but maybe that's a separate question (I don't want to get into it if it is going to be contraversial.)

Actions #5

Updated by Lucas Di Pentima about 6 years ago

Updates at 19ee2e029
Test run: https://ci.curoverse.com/job/developer-run-tests/635/ (waiting for workers atm)

Don't show X-Request-Id message on command startup.
Only show the request id once when logging error or debug messages.
Related tests added.

Actions #6

Updated by Peter Amstutz about 6 years ago

Lucas Di Pentima wrote:

Updates at 19ee2e029
Test run: https://ci.curoverse.com/job/developer-run-tests/635/ (waiting for workers atm)

Don't show X-Request-Id message on command startup.
Only show the request id once when logging error or debug messages.
Related tests added.

LGTM, please merge.

Actions #7

Updated by Lucas Di Pentima about 6 years ago

  • Status changed from In Progress to Resolved
Actions #8

Updated by Tom Morris over 5 years ago

  • Release set to 17
Actions

Also available in: Atom PDF