Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): support regex global flag in urlMatches #2560

Merged
merged 2 commits into from
Oct 27, 2021

Conversation

moander
Copy link
Contributor

@moander moander commented Oct 22, 2021

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

@moander moander requested a review from a team October 22, 2021 10:27
@obecny
Copy link
Member

obecny commented Oct 22, 2021

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

@moander
Copy link
Contributor Author

moander commented Oct 22, 2021

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
Copy link

codecov bot commented Oct 22, 2021

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%) ⬇️

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dyladan dyladan added the bug Something isn't working label Oct 26, 2021
@moander
Copy link
Contributor Author

moander commented Oct 26, 2021

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
Copy link
Member

dyladan commented Oct 26, 2021

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
Copy link
Contributor Author

moander commented Oct 27, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants