WIP / DRAFT : replacing shimmer with mpwrapper #732
Conversation
obecny
requested review from
bg451,
dyladan,
markwolff,
mayurkale22,
OlivierAlbertini,
rochdev and
vmarchaud
as code owners
obecny
added
the
size/L
obecny
changed the title
Codecov Report
@@ Coverage Diff @@
## master #732 +/- ##
==========================================
- Coverage 92.64% 88.33% -4.31%
==========================================
Files 226 225 -1
Lines 10231 10220 -11
Branches 939 920 -19
==========================================
- Hits 9478 9028 -450
- Misses 753 1192 +439
Continue to review full report at Codecov.
|
|
What value does mpwrapper provide over a small custom monkey-patch utility we might write ourselves? My sense is that if we can reduce the number of NPM dependencies that's good from a security perspective. |
@obecny wrote mpwrapper. I think the idea is to just test it out and maybe eventually move it into the otel repo |
|
I would prefer to have it inside the repo (and so inside this PR) so we can review both the modifications made to otel and the code of |
|
A few issues I have with mpWrapper:
function inputsHasErrors(
obj: any,
name: string,
wrapper: Function
): boolean {
if (typeof obj !== 'object') {
logger('module cannot be undefined');
return true;
}
if (typeof name !== 'string') {
logger(`expected name to be type 'string', received '${typeof name}'`);
return true;
}
if (!obj[name]) {
logger(`no original function ${name} to wrap`);
return true;
}
if (typeof obj[name] !== 'function') {
logger(`expected obj.${name} to be type 'function', received '${typeof obj[name]}'`);
return true;
}
if (typeof wrapper !== 'function') {
logger(`expected wrapper to be type 'function', received '${typeof wrapper}'`);
return true;
}
return false;
} |
|
I talked with @Flarna and he identified a few technical issues. Keep in mind that shimmer shares most if not all of these shortcomings as well.
import { wrap } from "mpwrapper"
function myFunc(original: Function) {
return original;
}
class Base {
foo() { return 'foo' }
}
class Derived extends Base {
bar() { return 'bar' }
}
const d = new Derived();
d.constructor.prototype.hasOwnProperty("foo") // false
console.log( // ['constructor', 'bar']
Object.getOwnPropertyNames(Derived.prototype)
);
wrap(Derived.prototype, "foo", myFunc);
// here we have changed the properties on `Derived.prototype`
d.constructor.prototype.hasOwnProperty("foo") // true
console.log( // ['constructor', 'bar', 'foo']
Object.getOwnPropertyNames(Derived.prototype)
);I think there are a few use cases that
|
yes the library is not a final so it doesn't have yet docs and unit tests - the idea was to show that with regards to this
So here it is the library which you can safely unpatch or patch whenever you want and you don't have
it's exactly the same what shimmer has, but we can always change it
you can always improve things
not it will not - neither shimmer nor mpwrapper will remove each other. But mpwrapper will always be able to remove what it had added. Shimmer saves the reference to "original" and original is the reference for storing all the callbacks in mpwrapper.
why would you even want such feature - string is good enough for what it does - it just adds information that it ha been patched, you can't write bullet proof solution that will prevent overwriting the same property by any1 else it's js you can overwrite everything.
yes it doesn't extend all possible thing, but it extent already more then shimmer, adding few more things it just a matter of adding them if we need to - so far it doesn't seems necessary otherwise it would already fail.
the same as previous it doesn't because we don't need that yet, we can easily add it if we need to
as previously just add more things which should be copied
here again, where do we need this ?, and if that is the case why not add that ?
we already use this functionality from shimmer, aren't we? Moreover as you can see I have already simplified the BasePlugin so that you don't need to implement unpatch in most cases unless you want to add some extra step. Instead of adding forced limits I would rather let user decide what he needs instead of limiting this from the very beginning without really a reason because technology allows that and implementation is already done.
we will have this problem and any1 that will use the opentelemetry will face this problem too. It will happen in moment when someone is already using a shimmer and we will keep on using shimmer too. But we can be at least safe and give people ability to switch from shimmer to mpwrapper if they face such problem - the solution will be there.
This has been already used by grpc and mongo |
|
I tried to unpatch a function first patched with mpwrapper and then with shimmer but it doesn't work: const shimmer = require("shimmer");
const mpwrapper = require("mpwrapper");
function theOriginal(a, b) {
console.log("the original", a, b);
}
const obj = {
theOriginal
};
const mpWrapRc = mpwrapper.wrap(obj, "theOriginal", (orig) => {
return function mpWrapper() {
console.log("mpWrapper start")
return orig.apply(this, arguments);
}
});
shimmer.wrap(obj, "theOriginal", (orig) => {
return function shimmerWrapper() {
console.log("shimmerWrapper start")
return orig.apply(this, arguments);
}
});
obj.theOriginal(1, 2);
console.log("\nnow unwrap and call again:")
mpWrapRc.unwrap();
obj.theOriginal(3, 4);results in Root cause is that |
good catch, for removing callbacks I will pass the wrapper as well and this way the callback will be removed |
|
To illustrate the issue with adding a function on prototype of derived class: const mpwrapper = require("mpwrapper");
class Base {
foo() {
console.log("foo")
}
}
class Derived extends Base {
bar() {
console.log("bar")
}
}
// if Base.prototype is patched here it's fine
mpwrapper.wrap(Derived.prototype, "foo", (orig) => {
return function() {
console.log("patched Derived#foo")
return orig.apply(this, arguments)
}
})
mpwrapper.wrap(Base.prototype, "foo", (orig) => {
return function() {
console.log("patched Base#foo")
return orig.apply(this, arguments)
}
})
const d = new Derived();
d.foo();results in ==> The second patch is omitted.
If it is not needed then I recommend to disallow it. If someone tries to patch a function which is not an own property then reject this as not supported and the user should adapt the code to select the base class. Depending on the complexity of the plugins added we may end up in the need for a more advance handling. |
|
this seems to be outdated and unwanted, closing |
Which problem is this PR solving?
Short description of the changes
shimmerunpatchfor BasePlugin tooEDIT: It is to validate it, test it and see if we want to keep it or leave shimmer - having in mind it cons