Remove custom arguments in object.__new__ of BaseInstrumentor

[rbagd]

Description

Classes inheriting from BaseInstrumentor class are singletons tracked by _instance attribute and this piece of code

    def __new__(cls, *args, **kwargs):
        if cls._instance is None:
            cls._instance = object.__new__(cls, *args, **kwargs)

If an instrumentor child class with parameters, e.g. SystemMetricsInstrumentor is initialized first, then these parameters are also passed to object.__new__ which results in an error as in #1438.

# This works
SystemMetricsInstrumentor()
SystemMetricsInstrumentor(config={})

# This does not
SystemMetricsInstrumentor(config={})
SystemMetricsInstrumentor()

Given the answers in this SO question, I am inclined to think that it's not needed to pass arguments to object.__new__ and they can be safely removed. Those arguments will still end up in the __init__ call.

Fixes #1438

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Added a unit test in system metrics to reproduce the issue (this requires singleton class to be reset during test run)
• Additional tests were ran with toxfor multiple other instrumented packages

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[ocelotl]

Nice, please add another test case, one that pretty much does this and checks no exception is raised:

def test_some_test_case_name(self):
    try:
        SystemMetricsInstrumentor()
        SystemMetricsInstrumentor(config={})
    except Exception as error:
        self.fail(f"Unexpected exception {error} raised")
    # do any necessary resetting
        try:
        SystemMetricsInstrumentor()
        SystemMetricsInstrumentor(config={})
    except Exception as error:
        self.fail(f"Unexpected exception {error} raised")

[rbagd]

I added an extra case as per @ocelotl request. I assumed he meant the init order should be inverted in the second case. 😉

[ocelotl]

I added an extra case as per @ocelotl request. I assumed he meant the init order should be inverted in the second case. wink

right! 😅 some tests are failing, please take a look

[rbagd]

All failing tests I've checked were related to #1460. Now that it got merged, I think it should be alright.

[rbagd]

Both failing jobs failed due to some issue with git itself. Is it possible to re-run just those two?

/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +95327fbe060f4136c197c59869fb13484d3b77c8:refs/remotes/pull/1439/merge
Error: fatal: protocol error: bad pack header
Error: The process '/usr/bin/git' failed with exit code 128

[srikanthccv]

Both failing jobs failed due to some issue with git itself. Is it possible to re-run just those two?

This should be fine now.

[srikanthccv]

@ocelotl PTAL

[rbagd]

@ocelotl @srikanthccv Linting failed due to a style change in black. I think I got it fixed so if possible to get this merged before they update it again, it'd be great. :)

[srikanthccv]

@ocelotl PTAL