For unknown reason this line
https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-exporter-prometheus/src/prometheus.ts#L150
doesn't work correctly in compiled version.
Any idea why and what can be missing ?
It is always false? The sum one I assume works though?
yes the sum works fine
can you verify if that doesn't work for you too ?
Sure. You have a simple code sample I can run or should I just make one?
not yet working on something in contrib repo so the PR should be soon. I cannot reproduce it with the example that is for observer in core repo :/. Was wondering if anyone already happen to have the same. Maybe if the number of observers is bigger, strange
although I'm using typescript now and example in core is in js
can show you on screenshare :)
I have some theory. In the new package I'm using the api and sdk. Then prometheus is also using the api and sdk. Then I'm using lerna to bootstrap my new package. This is causing that prometheus is using different api file then the new sdk. This is causing the instanceof to return false even though the classes looks the same, but the are different files in fact. Based on this I'm wondering that we should probably avoid comparing any data that might come from "outside" with the instanceof. Instead we should try to use
constructor.name === 'ObserverAggregator'
Oh yes that could definitely cause it. I typically try to avoid constructor name checking though, as this can cause problems with certain build systems and minification. Since we control this class, I wonder if we should just introduce an enum that we can check against.
I could make the change in my PR there https://github.com/open-telemetry/opentelemetry-js/pull/1038 since i've already rewrote that part
Done in https://github.com/vmarchaud/opentelemetry-js/commit/6dd4cc1b1493543a9babf6abf684bd5f7d7e7acd, if you could review that
I think this one can be closed @obecny ?
Most helpful comment
I could make the change in my PR there https://github.com/open-telemetry/opentelemetry-js/pull/1038 since i've already rewrote that part