open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.57k stars 743 forks source link

Allow instrumentations to be extendable #2620

Open bradfrosty opened 2 years ago

bradfrosty commented 2 years ago

Is your feature request related to a problem? Please describe.

It is currently difficult as a consumer to extend functionality to the automatic instrumentations. The visibility of most methods are private. If I want to fix something locally or add something custom, I need to copy and paste the existing source or force typescript to ignore it.

Describe the solution you'd like

I would like the default visibility of the private methods to beprotected, unless there is a compelling reason for it to be private.

Describe alternatives you've considered

To workaround this now, I am maintaining my own implementation of the instrumentations I want to extend. This is not ideal since I mostly using the existing implementation, with minor tweaks or fixes while waiting for the bug to be fixed or feature to be added.

Additional context

At minimum, I've been creating issues, but I try to contribute whatever I'm patching locally. However, since there is often a delay for these to get released, I would like the ability to use a local version until the next upgrade especially as a contributor.

dyladan commented 2 years ago

The current reasoning for having most methods private is that once they are made protected they are considered a part of the public API. Private methods can be changed or removed without warning in bugfix releases. If we make them protected it increases the API surface and in turn the maintenance burden. IMO it is usually better to upstream changes as new configuration options.

Can I ask which plugin you're wanting to extend and with what feature?

dyladan commented 2 years ago

Just so you know, it is possible to call private methods by using the obj["property"] syntax

class A {
    private foo() {
        return "foo"
    }
}

class B extends A {
    private bar() {
        console.log('calling foo');
        return this["foo"](); // this is allowed
    }
}
bradfrosty commented 2 years ago

That makes sense regarding the public API. I am currently wanting to use the fix we're discussing in this issue https://github.com/open-telemetry/opentelemetry-js-contrib/issues/548.

I didn't realize that was possible, so that makes things a little easier. However, how would I override a method that is called internally by another private method that I don't need to modify?

For example:

class A {
    private foo() {
        return "foo"
    }

    private getFoo() {
        return foo();
    }
}

class B extends A {
    private foo() {
        return 'FOO';
    }
}
dyladan commented 2 years ago

Thats a little trickier. Best i've been able to come up with is this:

class A {
    private foo() {
        return "foo"
    }

    public getFoo() {
        return this.foo();
    }
}

class B extends A {
    constructor() {
        super();
        this["foo"] = () => "FOO";
    }
}

new B().getFoo() //?
dyladan commented 2 years ago

Or if you want to make it a little clearer what your intent is:

class A {
    private foo() {
        return "foo"
    }

    public getFoo() {
        return this.foo();
    }
}

class B extends A {
    constructor() {
        super();
        this["foo"] = this.override_foo;
    }

    private override_foo() {
        return "FOO"
    }
}

new B().getFoo()
github-actions[bot] commented 2 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.