Skip to content

Make tracePropagationTargets matching case-insensitive #16018

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

Open
Lms24 opened this issue Apr 9, 2025 · 4 comments
Open

Make tracePropagationTargets matching case-insensitive #16018

Lms24 opened this issue Apr 9, 2025 · 4 comments
Labels
Meta: Breaking Package: core Issues related to the Sentry Core SDK
Milestone

Comments

@Lms24
Copy link
Member

Lms24 commented Apr 9, 2025

Problem Statement

Currently, we have a bit of an obscure problem with tracePropagationTargets:

If users make a fetch request to a URL with casing (for whatever reason), they likely apply the same casing when defining tracePropagationTargets:

Sentry.init({
  tracePropagationTargets: ['myApi.com', /^myApi.com\/.*/]
})

fetch('https://myApi.com');

Unfortunately, the values in tracePropagationTargets counter-intuitively don't match because we create a new URL() from the URL passed to fetch when determining if our tracing headers should be attached.

The constructed URL object converts the entered url though to all-lower-case, meaning that when matching myapi.com against both, the string or the regex, we wouldn't get a positive match. Therefore, the currently correct way of specifying TPT for this URL would any one of :

Sentry.init({
  tracePropagationTargets: [
     'myapi.com', 
     /^myapi.com/,
     /^myApi.com/i,
]
})

Solution Brainstorm

For simplicity, we propose to convert all entries in tracePropagationTargets .toLower() and add a case insensitivity /i flag to RegExp values.

We're a bit worried though about subtly breaking existing setups that perhaps rely on case sensitivity. Therefore we consider this behaviour-breaking and opt to make the change in the next major.
(Also, intuitively, this sounds like a niche/edge case as it's rather uncommon to capitalize URLs)

@Lms24 Lms24 added Meta: Breaking Package: core Issues related to the Sentry Core SDK labels Apr 9, 2025
@Lms24 Lms24 added this to the 10.0.0 milestone Apr 9, 2025
@Lms24 Lms24 changed the title Make tracePropagationTargets case-insensitive Make tracePropagationTargets matching case-insensitive Apr 9, 2025
@mydea
Copy link
Member

mydea commented Apr 9, 2025

We're a bit worried though about subtly breaking existing setups that perhaps rely on case sensitivity.

How would you depend on this behavior today? I just wonder if there is really a case where we would unintentionally break something vs. it suddenly starting to behave as I want? If a user has configured a trace propagation target myapi.com I find it extremely unlikely that they would expect/depend on requests to myApi.com not being propagated? And on the other hand, if they specifiy myApi.com it is obviously a bug that this will not match anything, basically.

So my POV is: this is a bug and was never supposed to be case insensitive, so I would just fix it 😅

@Lms24
Copy link
Member Author

Lms24 commented Apr 9, 2025

I think the concrete concern was places where it's more likely to actually encounter capitalized letters, like query params.

So maybe something like 'myApi/users?traceRequest=1'

though now that I think about it, this would only be a problem if we there was a specific all lower case param and a camel-cased version of the same parameter, so rather unlikely?

The overall concern: Right now, we (unexpectedly) would not attach headers to a variety of requests. Maybe users depend on this inderectly, in the sense that if we changed it, they'd get CORS errors because they didn't realize it was a problem before. So it's kinda a spacebar heating situation xD

Copy link
Member

mydea commented Apr 11, 2025

no strong feelings, we can also leave it for v10! I'll leave the decision to you ;)

@AbhiPrasad AbhiPrasad self-assigned this Apr 14, 2025
@AbhiPrasad
Copy link
Member

huh I didn't do this 😅

Linear weirdness?

@AbhiPrasad AbhiPrasad reopened this Apr 14, 2025
@AbhiPrasad AbhiPrasad removed their assignment Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meta: Breaking Package: core Issues related to the Sentry Core SDK
Projects
None yet
Development

No branches or pull requests

3 participants