Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2106953006:
adding os metrics (os name and version) being collected every hour (Closed)
Descriptionadding os metrics (os name and version) being collected every hour
BUG=623856
Committed: https://chromium.googlesource.com/infra/infra/+/a4169dbfef9b2e6f43dd6fa655e67ffe090219d0
Patch Set 1 #
Total comments: 17
Patch Set 2 : adding os metrics (os name and version) being collected every hour #
Total comments: 12
Patch Set 3 : adding os metrics (os name and version) being collected every hour #Patch Set 4 : adding os metrics (os name and version) being collected every hour #
Total comments: 5
Patch Set 5 : adding comment explaining minute guarantee #
Total comments: 2
Patch Set 6 : fixing metric comments #Patch Set 7 : adding test cases to cover missing lines added in this CR #Patch Set 8 : updating system_metrics.clear_os_info to clear all added fields, fixing unit tests for clear #
Messages
Total messages: 50 (19 generated)
chrishall@chromium.org changed reviewers: + dsansome@chromium.org
May I also ask to collect kernel architecture (platform.machine()) in python) and Python interpreter bitness (can be determined by checking sys.maxsize)? (We have a bunch of Linux machines with 64-bit kernels and 32-bit userland, thus checking python bitness).
https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:27: # SysMon.task is called every minute Wrap these comments to 80 characters? https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:31: self._call_count = -1 I'd start at 0 and move the += 1 into the finally block in task() https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:39: self._call_count %= 60 If this is named call_count I think it makes sense to just have it be a raw count, and do the % 60 in is_hour instead https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:64: self.count_call() I'm not sure hiding the implementations of these inside methods is any more readable than just having self._call_count += 1 here, and (self._call_count % 60) == 0 below. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... File infra/services/sysmon/system_metrics.py (right): https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... infra/services/sysmon/system_metrics.py:90: description='OS name on the machine ' Align these lines with the ' above https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... infra/services/sysmon/system_metrics.py:176: def get_os_info(): Please add tests for this to test/system_metrics_test.py. You'll probably need to @mock.patch('platform.system') and friends. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... infra/services/sysmon/system_metrics.py:216: Need 2 blank lines between top-level functions
Description was changed from ========== adding os metrics (os name and version) being collected every hour BUG= ========== to ========== adding os metrics (os name and version) being collected every hour BUG=620610 ==========
Description was changed from ========== adding os metrics (os name and version) being collected every hour BUG=620610 ========== to ========== adding os metrics (os name and version) being collected every hour BUG=623856 ==========
ddoman@chromium.org changed reviewers: + ddoman@chromium.org
https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:43: return self._call_count == 0 Doesn't self.opts.interval determine how often task() gets called?
On 2016/06/29 07:57:55, ddoman wrote: > https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... > File infra/services/sysmon/__main__.py (right): > > https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... > infra/services/sysmon/__main__.py:43: return self._call_count == 0 > Doesn't self.opts.interval determine how often task() gets called? yes, we want most of the metrics within task to be collected every minute - but some metrics (currently only the OS metrics) to be collected at a coarser granularity.
https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:43: return self._call_count == 0 On 2016/06/29 07:57:54, ddoman wrote: > Doesn't self.opts.interval determine how often task() gets called? You should add a comment here explaining why we're only doing this once an hour.
On 2016/06/29 06:18:37, Vadim Sh. wrote: > May I also ask to collect kernel architecture (platform.machine()) in python) > and Python interpreter bitness (can be determined by checking sys.maxsize)? > > (We have a bunch of Linux machines with 64-bit kernels and 32-bit userland, thus > checking python bitness). I am happy to add in a proc/os/arch capturing platform.machine() - 'x86_64' on my machine. Whereas sys.maxsize reports 9223372036854775807 (2**63 - 1) on my machine, which makes for a crappy metric as-is. Googling around it seems this is commonly done via `sys.maxsize > 2**32` to check for 64bit-ed-ness. Is this sufficient?
On 2016/06/30 04:50:17, chrishall wrote: > On 2016/06/29 06:18:37, Vadim Sh. wrote: > > May I also ask to collect kernel architecture (platform.machine()) in python) > > and Python interpreter bitness (can be determined by checking sys.maxsize)? > > > > (We have a bunch of Linux machines with 64-bit kernels and 32-bit userland, > thus > > checking python bitness). > > I am happy to add in a proc/os/arch capturing platform.machine() - 'x86_64' on > my machine. > > Whereas sys.maxsize reports 9223372036854775807 (2**63 - 1) on my machine, which > makes for a crappy metric as-is. > Googling around it seems this is commonly done via `sys.maxsize > 2**32` to > check for 64bit-ed-ness. > Is this sufficient? Yep. That's what I had in mind. Thanks.
chrishall@chromium.org changed reviewers: + vadimsh@chromium.org
https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:27: # SysMon.task is called every minute On 2016/06/29 06:21:29, dsansome wrote: > Wrap these comments to 80 characters? Done. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:31: self._call_count = -1 On 2016/06/29 06:21:29, dsansome wrote: > I'd start at 0 and move the += 1 into the finally block in task() Done. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:39: self._call_count %= 60 On 2016/06/29 06:21:29, dsansome wrote: > If this is named call_count I think it makes sense to just have it be a raw > count, and do the % 60 in is_hour instead Done. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:43: return self._call_count == 0 On 2016/06/29 07:57:54, ddoman wrote: > Doesn't self.opts.interval determine how often task() gets called? Done. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/__mai... infra/services/sysmon/__main__.py:64: self.count_call() On 2016/06/29 06:21:29, dsansome wrote: > I'm not sure hiding the implementations of these inside methods is any more > readable than just having self._call_count += 1 here, and (self._call_count % > 60) == 0 below. I disagree, I wanted to give them names rather than have 'random math' in the try/finally below.
https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... File infra/services/sysmon/system_metrics.py (right): https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... infra/services/sysmon/system_metrics.py:90: description='OS name on the machine ' On 2016/06/29 06:21:29, dsansome wrote: > Align these lines with the ' above Done. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... infra/services/sysmon/system_metrics.py:176: def get_os_info(): On 2016/06/29 06:21:29, dsansome wrote: > Please add tests for this to test/system_metrics_test.py. You'll probably need > to @mock.patch('platform.system') and friends. Done. https://codereview.chromium.org/2106953006/diff/1/infra/services/sysmon/syste... infra/services/sysmon/system_metrics.py:216: On 2016/06/29 06:21:29, dsansome wrote: > Need 2 blank lines between top-level functions Done.
lgtm https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/_... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/_... infra/services/sysmon/__main__.py:32: # should be called at the end of each call to self.task Make this a docstring? https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/s... File infra/services/sysmon/system_metrics.py (right): https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/s... infra/services/sysmon/system_metrics.py:185: Add an extra \n here https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/s... infra/services/sysmon/system_metrics.py:187: os_name_data = "" We seem to use single quotes elsewhere in this file: s/"/'/? https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... File infra/services/sysmon/test/system_metrics_test.py (right): https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... infra/services/sysmon/test/system_metrics_test.py:177: self.AssertTrue(platform_system_mock.called) Does this work? The methods are named with a lower-case first letter https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... infra/services/sysmon/test/system_metrics_test.py:177: self.AssertTrue(platform_system_mock.called) Also platform_system_mock.assert_called_once_with()
https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/_... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/_... infra/services/sysmon/__main__.py:32: # should be called at the end of each call to self.task On 2016/07/01 04:30:56, dsansome wrote: > Make this a docstring? Done. https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/s... File infra/services/sysmon/system_metrics.py (right): https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/s... infra/services/sysmon/system_metrics.py:185: On 2016/07/01 04:30:56, dsansome wrote: > Add an extra \n here Done. https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/s... infra/services/sysmon/system_metrics.py:187: os_name_data = "" On 2016/07/01 04:30:56, dsansome wrote: > We seem to use single quotes elsewhere in this file: s/"/'/? Done. https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... File infra/services/sysmon/test/system_metrics_test.py (right): https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... infra/services/sysmon/test/system_metrics_test.py:177: self.AssertTrue(platform_system_mock.called) On 2016/07/01 04:30:56, dsansome wrote: > Does this work? The methods are named with a lower-case first letter Done. https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... infra/services/sysmon/test/system_metrics_test.py:177: self.AssertTrue(platform_system_mock.called) On 2016/07/01 04:30:56, dsansome wrote: > Does this work? The methods are named with a lower-case first letter Done. https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... infra/services/sysmon/test/system_metrics_test.py:177: self.AssertTrue(platform_system_mock.called) On 2016/07/01 04:30:56, dsansome wrote: > Also platform_system_mock.assert_called_once_with() Done. https://codereview.chromium.org/2106953006/diff/20001/infra/services/sysmon/t... infra/services/sysmon/test/system_metrics_test.py:177: self.AssertTrue(platform_system_mock.called) On 2016/07/01 04:30:56, dsansome wrote: > Also platform_system_mock.assert_called_once_with() very nice!
NB: currently unable to run tests on my local machine:
~/devel/I/infra$ ./test.py test
infra/services/sysmon/test/system_metrics_test.py
Running infra/services/sysmon/test/system_metrics_test.py...
usage: expect_tests [-h] {debug,test,train,list} ...
expect_tests: error: unrecognized arguments:
--coveragerc=/usr/local/google/home/chrishall/devel/I/infra/infra/services/sysmon/test/system_metrics_test.py/.coveragerc
--html-report-subdir=infra/services/sysmon/test/system_metrics_test.py
Tests failed in modules:
infra/services/sysmon/test/system_metrics_test.py
For detailed coverage report and per-line branch coverage,
rerun with --html-report <dir>
chrishall@chrishall-z440:~/devel/I/infra$
`gclient sync` solved it (didn't seem to earlier, but today it worked). Had to rejig tests due to some weirdness within mock. Tests now pass for me. Submitting.
chrishall@google.com changed reviewers: + chrishall@google.com, vadimsh@google.com - ddoman@chromium.org, dsansome@chromium.org, vadimsh@chromium.org
vadimsh can you please review this patch. I need OWNERS approval.
vadimsh@chromium.org changed reviewers: + vadimsh@chromium.org - vadimsh@google.com
https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... infra/services/sysmon/__main__.py:83: self.count_minute() I don't think task() is guaranteed to be called once per minute. I see '--interval' above, which is 10 sec by default. I think the minute comes from here: https://chrome-internal.googlesource.com/infra/puppet/+/master/puppetm/etc/pu... (And it's not part of the sysmon source code, so mentioning 'minute' in the source code is confusing). Either rename variables to something else, or use real time to detect 60min.
https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... infra/services/sysmon/__main__.py:83: self.count_minute() On 2016/07/06 19:36:54, Vadim Sh. wrote: > I don't think task() is guaranteed to be called once per minute. > > I see '--interval' above, which is 10 sec by default. I think the minute comes > from here: > https://chrome-internal.googlesource.com/infra/puppet/+/master/puppetm/etc/pu... > > (And it's not part of the sysmon source code, so mentioning 'minute' in the > source code is confusing). > > Either rename variables to something else, or use real time to detect 60min. The guarantee to be called every 60s comes from the line you linked. Hmm I am unhappy with either suggestion. I think renaming them to anything similar to "is_60th_call' would be strictly worse off; the intent here is to capture the data every hour - and the current configuration should give us that. Doing date/time math seems more expensive and less accurate. Unless this code can somehow access that configured interval and use that in the calculation. The reason we are doing this is to try optimise for space, in particular the space consumed by the data kept live in memory.
lgtm, since it's all nits anyway https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... infra/services/sysmon/__main__.py:50: help='time (in seconds) between sampling system metrics') at least change the default here to 60, so the reader has some idea where "minute" comes from https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... infra/services/sysmon/__main__.py:83: self.count_minute() On 2016/07/13 05:49:33, chrishall wrote: > On 2016/07/06 19:36:54, Vadim Sh. wrote: > > I don't think task() is guaranteed to be called once per minute. > > > > I see '--interval' above, which is 10 sec by default. I think the minute comes > > from here: > > > https://chrome-internal.googlesource.com/infra/puppet/+/master/puppetm/etc/pu... > > > > (And it's not part of the sysmon source code, so mentioning 'minute' in the > > source code is confusing). > > > > Either rename variables to something else, or use real time to detect 60min. > > > The guarantee to be called every 60s comes from the line you linked. It's in another file, and the casual reader of this code may not know about it and would be wondering "where the minute guarantee comes from?". > > Hmm I am unhappy with either suggestion. > > I think renaming them to anything similar to "is_60th_call' would be strictly > worse off; the intent here is to capture the data every hour - and the current > configuration should give us that. > > Doing date/time math seems more expensive and less accurate. Performance overhead is negligible (a syscall + some math once per minute), and it would be in fact more accurate, since one iteration of the loop currently takes 60sec of sleep + time to execute 'task' itself (considering there's a flush there, it may be a second). So each iteration is in fact ~=61 sec and this one extra second accumulates during the course of an hour, so the flush will happen ~= each 61 min, not 60. Just saying... You've mentioned the accuracy first :) > > > Unless this code can somehow access that configured interval and use that in the > calculation. > > The reason we are doing this is to try optimise for space, in particular the > space consumed by the data kept live in memory.
On 2016/07/13 15:12:46, Vadim Sh. wrote: > lgtm, since it's all nits anyway Thanks. > https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... > File infra/services/sysmon/__main__.py (right): > > https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... > infra/services/sysmon/__main__.py:50: help='time (in seconds) between sampling > system metrics') > at least change the default here to 60, so the reader has some idea where > "minute" comes from I should probably add a comment at some point to this to clarify. > > > > I think renaming them to anything similar to "is_60th_call' would be strictly > > worse off; the intent here is to capture the data every hour - and the current > > configuration should give us that. > > > > Doing date/time math seems more expensive and less accurate. > Performance overhead is negligible (a syscall + some math once per minute), and > it would be in fact more accurate, since one iteration of the loop currently > takes 60sec of sleep + time to execute 'task' itself (considering there's a > flush there, it may be a second). So each iteration is in fact ~=61 sec and this > one extra second accumulates during the course of an hour, so the flush will > happen ~= each 61 min, not 60. > > Just saying... You've mentioned the accuracy first :) Good point, I was thinking of accuracy from the POV of 'number of calls between collection' (which counting calls will obviously be more accurate for) - as I was thinking of squashing every 60 collected points to a single point. but you're right in that checking time will be more accurate for 'time between each collection' - fair point.
https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... File infra/services/sysmon/__main__.py (right): https://codereview.chromium.org/2106953006/diff/60001/infra/services/sysmon/_... infra/services/sysmon/__main__.py:83: self.count_minute() On 2016/07/13 15:12:45, Vadim Sh. wrote: > On 2016/07/13 05:49:33, chrishall wrote: > > On 2016/07/06 19:36:54, Vadim Sh. wrote: > > > I don't think task() is guaranteed to be called once per minute. > > > > > > I see '--interval' above, which is 10 sec by default. I think the minute > comes > > > from here: > > > > > > https://chrome-internal.googlesource.com/infra/puppet/+/master/puppetm/etc/pu... > > > > > > (And it's not part of the sysmon source code, so mentioning 'minute' in the > > > source code is confusing). > > > > > > Either rename variables to something else, or use real time to detect 60min. > > > > > > The guarantee to be called every 60s comes from the line you linked. > It's in another file, and the casual reader of this code may not know about it > and would be wondering "where the minute guarantee comes from?". > > > > Hmm I am unhappy with either suggestion. > > > > I think renaming them to anything similar to "is_60th_call' would be strictly > > worse off; the intent here is to capture the data every hour - and the current > > configuration should give us that. > > > > Doing date/time math seems more expensive and less accurate. > Performance overhead is negligible (a syscall + some math once per minute), and > it would be in fact more accurate, since one iteration of the loop currently > takes 60sec of sleep + time to execute 'task' itself (considering there's a > flush there, it may be a second). So each iteration is in fact ~=61 sec and this > one extra second accumulates during the course of an hour, so the flush will > happen ~= each 61 min, not 60. > > Just saying... You've mentioned the accuracy first :) > > > > > > Unless this code can somehow access that configured interval and use that in > the > > calculation. > > > > The reason we are doing this is to try optimise for space, in particular the > > space consumed by the data kept live in memory. > Done.
The CQ bit was checked by chrishall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dsansome@chromium.org, vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2106953006/#ps80001 (title: "adding comment explaining minute guarantee")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)
The CQ bit was checked by chrishall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)
dsansome@chromium.org changed reviewers: + dsansome@chromium.org
https://codereview.chromium.org/2106953006/diff/80001/infra/services/sysmon/s... File infra/services/sysmon/system_metrics.py (right): https://codereview.chromium.org/2106953006/diff/80001/infra/services/sysmon/s... infra/services/sysmon/system_metrics.py:101: 'metric:hostname.') You can remove 'metric:hostname' from the description, since you're not setting that field any more.
https://codereview.chromium.org/2106953006/diff/80001/infra/services/sysmon/s... File infra/services/sysmon/system_metrics.py (right): https://codereview.chromium.org/2106953006/diff/80001/infra/services/sysmon/s... infra/services/sysmon/system_metrics.py:101: 'metric:hostname.') On 2016/07/21 01:55:25, dsansome wrote: > You can remove 'metric:hostname' from the description, since you're not setting > that field any more. oops, derp!
The CQ bit was checked by chrishall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org, dsansome@chromium.org Link to the patchset: https://codereview.chromium.org/2106953006/#ps120001 (title: "adding test cases to cover missing lines added in this CR")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Mac Tester on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Mac%20Tester/bu...) Infra Win Tester (Swarming) on master.tryserver.infra (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/304371790e64e110)
The CQ bit was checked by chrishall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org, dsansome@chromium.org Link to the patchset: https://codereview.chromium.org/2106953006/#ps140001 (title: "updating system_metrics.clear_os_info to clear all added fields, fixing unit tests for clear")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== adding os metrics (os name and version) being collected every hour BUG=623856 ========== to ========== adding os metrics (os name and version) being collected every hour BUG=623856 Committed: https://chromium.googlesource.com/infra/infra/+/a4169dbfef9b2e6f43dd6fa655e67... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/infra/infra/+/a4169dbfef9b2e6f43dd6fa655e67... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
