improv(instrumentation-http): supressInstrumentation when we get a request on ignoredPath [#1831]

[vmarchaud]

Fixes #1831

[codecov]

Codecov Report

Merging #1838 (1c2ab82) into master (f43855d) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1838      +/-   ##
==========================================
- Coverage   92.60%   92.41%   -0.20%     
==========================================
  Files         174      170       -4     
  Lines        6048     5734     -314     
  Branches     1288     1236      -52     
==========================================
- Hits         5601     5299     -302     
+ Misses        447      435      -12     

Impacted Files	Coverage Δ
...ges/opentelemetry-instrumentation-http/src/http.ts	95.54% <100.00%> (+0.05%)	⬆️
packages/opentelemetry-plugin-http/src/http.ts	97.32% <100.00%> (+0.04%)	⬆️
...ntelemetry-web/src/enums/PerformanceTimingNames.ts	100.00% <0.00%> (ø)
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
...-instrumentation-fetch/src/enums/AttributeNames.ts
...mentation-xml-http-request/src/enums/EventNames.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
packages/opentelemetry-web/src/utils.ts	94.70% <0.00%> (+0.07%)	⬆️
...emetry-core/src/platform/node/RandomIdGenerator.ts	93.75% <0.00%> (+6.25%)	⬆️

[Flarna]

Maybe a simple regression test should be added to ensure that this doesn't get removed per accident.

[vmarchaud]

Maybe a simple regression test should be added to ensure that this doesn't get removed per accident.

That's why i added https://github.com/open-telemetry/opentelemetry-js/pull/1838/files#diff-ce862d1183e8ab52e6c94e5cc3022280fe213394a9b88e7f473f16620db344efR213, it create a "child" span within the request so if suppressInstrumentation isnt used, tests will fails since they expect to have 0 span.

[obecny]

I think added changes are not covered with unit tests.

[dyladan]

I think added changes are not covered with unit tests.

It is tested here https://github.com/open-telemetry/opentelemetry-js/pull/1838/files#diff-ce862d1183e8ab52e6c94e5cc3022280fe213394a9b88e7f473f16620db344efR213. Without this change, that change to the test would cause it to fail because it creates a span that would not be ignored as a child of an ignored path.

[obecny]

I think added changes are not covered with unit tests.

It is tested here https://github.com/open-telemetry/opentelemetry-js/pull/1838/files#diff-ce862d1183e8ab52e6c94e5cc3022280fe213394a9b88e7f473f16620db344efR213. Without this change, that change to the test would cause it to fail because it creates a span that would not be ignored as a child of an ignored path.

I would expect some simple test with checking that the span hasn't been exported, hard to say where is such unit test something like this it('should not export supressed span', ()=>{}); and after reverting the code this test should be failing.

No unit test has been added, just modification of before block, if anything fails it won't tell you why it failed - at least from all modified code this doesn't look so.

[vmarchaud]

No unit test has been added, just modification of before block, if anything fails it won't tell you why it failed - at least from all modified code this doesn't look so.

I don't agree, no unit test has been added because a test for the feature that the improv is on (when we ignore paths in the config) was already existing. I don't see the advantage of having multiple individual test for the same feature instead of one that test everything.

if anything fails it won't tell you why it failed

That's generally a "problem" with the tests we write, there are no description of each assert. It forces you to debug manually the test (one could argue that you must do this in every case), at which point it's easy to find out which span is exported.

[obecny]

That's generally a "problem" with the tests we write, there are no description of each assert. It forces you to debug manually the test (one could argue that you must do this in every case), at which point it's easy to find out which span is exported.

That is exactly what I'm telling, having a simple unit test as I described would be a perfect description and documentation of the changed piece of code. This is a 5 minute and will save much more minutes in future for any1 who will be modifying this part of code. Unit test will fail and it will tell you exactly why.

[vmarchaud]

That is exactly what I'm telling, having a simple unit test as I described would be a perfect description and documentation of the changed piece of code. This is a 5 minute and will save much more minutes in future for any1 who will be modifying this part of code. Unit test will fail and it will tell you exactly why.

I think you misundestood what i said, i meant that it's much better to document what assert are supposed to test instead of having multiple "simple" unit test