fix(core): support regex global flag in urlMatches

[moander]

RegExp with global flag is problematic when the rx is persisted. A common place for this bug to appear is in options.

Example:

registerInstrumentations({
  instrumentations: [
    getWebAutoInstrumentations({
      '@opentelemetry/instrumentation-xml-http-request': {
        ignoreUrls: [/com/g],
      },
    }),
  ],
});

The above example will ignore the first request but not the second one.

Short description of the changes

!!str.match(rx) instead of rx.test(str) solves the issue.

Match is a tiny bit slower than test. If test is preferred the alternative solution is to rx.lastIndex = 0 after each test.

Type of change

Please delete options that are not relevant.

• [ X ] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Reproduced by adding a test to url.tests.ts

Try this to verify

const { isUrlIgnored } = require('@opentelemetry/core');

const ignoredUrls = [/test/g];

console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));

Output

true
false
true
false
true
false

Checklist:

• [ X ] Followed the style guidelines of this project
• [ X ] Unit tests have been added

[obecny]

can you be more specific about the bug you are trying to fix ? as this was already changed in past
#2226

[moander]

can you be more specific about the bug you are trying to fix ? as this was already changed in past #2226

const { isUrlIgnored } = require('@opentelemetry/core');

const ignoredUrls = [/test/g];

console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));
console.log(isUrlIgnored('test.com', ignoredUrls));

Output

true
false
true
false
true
false

[codecov]

Codecov Report

Merging #2560 (e30984e) into main (c1939a7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2560      +/-   ##
==========================================
- Coverage   93.09%   93.07%   -0.02%     
==========================================
  Files         140      140              
  Lines        5172     5172              
  Branches     1111     1111              
==========================================
- Hits         4815     4814       -1     
- Misses        357      358       +1     

Impacted Files	Coverage Δ
packages/opentelemetry-core/src/utils/url.ts	100.00% <100.00%> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts	87.50% <0.00%> (-6.25%)	⬇️

[dyladan]

Thanks for the example. I'm curious, can you explain why it is broken with the old implementation? From what I see it should work just fine unless I am misunderstanding .test function.

[moander]

Thanks for the example. I'm curious, can you explain why it is broken with the old implementation? From what I see it should work just fine unless I am misunderstanding .test function.

It's broken because it's nondeterministic. Functions like this should be pure without any side effects.

[dyladan]

Right I get that it's nondeterministic, but why is it nondeterministic? Nothing in the old implementation would have made me think it wasn't deterministic.

export function urlMatches(url: string, urlToMatch: string | RegExp): boolean {
  if (typeof urlToMatch === 'string') { // this branch seems obviously deterministic
    return url === urlToMatch;
  } else { // this branch is `RegExp.prototype.test(url: string)` which I would also think is deterministic
    return urlToMatch.test(url);
  }
}

[moander]

Right I get that it's nondeterministic, but why is it nondeterministic? Nothing in the old implementation would have made me think it wasn't deterministic.

The reason is that the g flag works like a cursor. Instead of parsing the whole thing and return a full response like .match the .test (and .exec) will stop on first match. Then the next time it continues looking for the second and so on.

I agree It is very unexpected. The top 3 in JS is this one followed by 'OOO'.replace('O', 'F') == 'FOO' and 0.1+0.2 !== 0.3 in my opinion.