feat(browser): Set user.ip_address explicitly to {{auto}} - #15008

Merged
mydea merged 6 commits into
developfrom
fn/set-user-ip-browser
Jan 20, 2025
Merged

feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom
fn/set-user-ip-browser

Conversation

@mydea

@mydeamydea commented Jan 14, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Part of getsentry/team-sdks#118

This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.
So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)

This is set for:

  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)

@mydeamydea self-assigned this Jan 14, 2025
@mydeamydea mentioned this pull request Jan 14, 2025
Open
@github-actions

github-actionsBot commented Jan 14, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺

View base workflow run

@codecov

codecovBot commented Jan 14, 2025•
edited
Loading

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan

Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)

Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@krystofwoldrich

krystofwoldrich commented Jan 14, 2025

Copy link
Copy Markdown
Contributor

Why do we implement this only in browser client and not in the core/baseClient?

@timfish

timfish commented Jan 14, 2025•
edited
Loading

Copy link
Copy Markdown
Collaborator

Why do we implement this only in browser client and not in the core/baseClient?

Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.

I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?

This PR is missing the sendDefaultPii check!

@mydea
mydeaforce-pushed the fn/set-user-ip-browser branch from 8670611 to fb62082CompareJanuary 15, 2025 08:48
@mydea

mydea commented Jan 15, 2025

Copy link
Copy Markdown
MemberAuthor

The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔

Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!

@mydea
mydeaforce-pushed the fn/set-user-ip-browser branch from 522d43a to b34541aCompareJanuary 15, 2025 14:08
@mydea

mydea commented Jan 16, 2025

Copy link
Copy Markdown
MemberAuthor

@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!

@mydea
mydeaforce-pushed the fn/set-user-ip-browser branch from b34541a to 22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea marked this pull request as ready for review January 17, 2025 11:15
@mydea
mydea requested a review from a team as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed the fn/set-user-ip-browser branch from 22b2da1 to e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich mentioned this pull request Jan 17, 2025
Closed
@mydea
mydeaforce-pushed the fn/set-user-ip-browser branch from e27c3a3 to b54f4c2CompareJanuary 17, 2025 13:18
This was referenced Jan 17, 2025
Closed
Closed
timfish
timfish approved these changes Jan 17, 2025
View reviewed changes

@timfishtimfish left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!

@mydea

mydea commented Jan 20, 2025

Copy link
Copy Markdown
MemberAuthor

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!

There is already this issue: #5347

@mydea
mydeaforce-pushed the fn/set-user-ip-browser branch from b54f4c2 to a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea merged commit eedf4a5 into developJan 20, 2025
@mydea
mydea deleted the fn/set-user-ip-browser branch January 20, 2025 12:28
This was referenced Jan 20, 2025
Merged
Merged
@mydeamydea mentioned this pull request Jan 20, 2025
Closed
krystofwoldrich added a commit that referenced this pull request Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric
+1 more reviewer
@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
feat(browser): Set user.ip_address explicitly to {{auto}} - #15008
Merged
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Jan 20, 2025
Merged
feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Conversation
@mydea
@mydeamydea
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Part of getsentry/team-sdks#118
This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.

So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)
This is set for:
  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)
@mydeamydea
self-assigned this
Jan 14, 2025
@mydeamydea
mentioned this pull request
Jan 14, 2025

Open

@github-actions
github-actionsBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺
View base workflow run
@codecov
codecovBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan
Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)
Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@krystofwoldrich
krystofwoldrich
commented
Jan 14, 2025

Copy link


Copy Markdown

Contributor
Why do we implement this only in browser client and not in the core/baseClient?
@timfish
timfish
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Collaborator
Why do we implement this only in browser client and not in the core/baseClient?
Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.
I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?
This PR is missing the sendDefaultPii check!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
8670611 to
fb62082CompareJanuary 15, 2025 08:48
@mydea
mydea
commented
Jan 15, 2025

Copy link


Copy Markdown

MemberAuthor
The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔
Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
522d43a to
b34541aCompareJanuary 15, 2025 14:08
@mydea
mydea
commented
Jan 16, 2025

Copy link


Copy Markdown

MemberAuthor
@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b34541a to
22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea
marked this pull request as ready for review
January 17, 2025 11:15
@mydea
mydea
requested a review
from a team
as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
22b2da1 to
e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich
mentioned this pull request
Jan 17, 2025

Closed

@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
e27c3a3 to
b54f4c2CompareJanuary 17, 2025 13:18

This was referenced Jan 17, 2025

Closed


Closed

timfish
timfish
approved these changes
Jan 17, 2025
View reviewed changes
@timfishtimfish
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
@mydea
mydea
commented
Jan 20, 2025

Copy link


Copy Markdown

MemberAuthor
We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
There is already this issue: #5347
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b54f4c2 to
a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea
merged commit eedf4a5
into
developJan 20, 2025
@mydea
mydea
deleted the

fn/set-user-ip-browser

branch
January 20, 2025 12:28

This was referenced Jan 20, 2025
 Merged

 Merged

@mydeamydea
mentioned this pull request
Jan 20, 2025

Closed

krystofwoldrich added a commit
that referenced
this pull request
Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric

+1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(browser): Set user.ip_address explicitly to {{auto}} - #15008
Merged
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Jan 20, 2025
Merged
feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Conversation
@mydea
@mydeamydea
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Part of getsentry/team-sdks#118
This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.

So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)
This is set for:
  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)
@mydeamydea
self-assigned this
Jan 14, 2025
@mydeamydea
mentioned this pull request
Jan 14, 2025

Open

@github-actions
github-actionsBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺
View base workflow run
@codecov
codecovBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan
Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)
Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@krystofwoldrich
krystofwoldrich
commented
Jan 14, 2025

Copy link


Copy Markdown

Contributor
Why do we implement this only in browser client and not in the core/baseClient?
@timfish
timfish
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Collaborator
Why do we implement this only in browser client and not in the core/baseClient?
Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.
I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?
This PR is missing the sendDefaultPii check!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
8670611 to
fb62082CompareJanuary 15, 2025 08:48
@mydea
mydea
commented
Jan 15, 2025

Copy link


Copy Markdown

MemberAuthor
The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔
Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
522d43a to
b34541aCompareJanuary 15, 2025 14:08
@mydea
mydea
commented
Jan 16, 2025

Copy link


Copy Markdown

MemberAuthor
@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b34541a to
22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea
marked this pull request as ready for review
January 17, 2025 11:15
@mydea
mydea
requested a review
from a team
as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
22b2da1 to
e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich
mentioned this pull request
Jan 17, 2025

Closed

@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
e27c3a3 to
b54f4c2CompareJanuary 17, 2025 13:18

This was referenced Jan 17, 2025

Closed


Closed

timfish
timfish
approved these changes
Jan 17, 2025
View reviewed changes
@timfishtimfish
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
@mydea
mydea
commented
Jan 20, 2025

Copy link


Copy Markdown

MemberAuthor
We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
There is already this issue: #5347
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b54f4c2 to
a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea
merged commit eedf4a5
into
developJan 20, 2025
@mydea
mydea
deleted the

fn/set-user-ip-browser

branch
January 20, 2025 12:28

This was referenced Jan 20, 2025
 Merged

 Merged

@mydeamydea
mentioned this pull request
Jan 20, 2025

Closed

krystofwoldrich added a commit
that referenced
this pull request
Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric

+1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(browser): Set user.ip_address explicitly to {{auto}} - #15008
Merged
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Jan 20, 2025
Merged
feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Conversation
@mydea
@mydeamydea
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Part of getsentry/team-sdks#118
This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.

So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)
This is set for:
  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)
@mydeamydea
self-assigned this
Jan 14, 2025
@mydeamydea
mentioned this pull request
Jan 14, 2025

Open

@github-actions
github-actionsBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺
View base workflow run
@codecov
codecovBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan
Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)
Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@krystofwoldrich
krystofwoldrich
commented
Jan 14, 2025

Copy link


Copy Markdown

Contributor
Why do we implement this only in browser client and not in the core/baseClient?
@timfish
timfish
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Collaborator
Why do we implement this only in browser client and not in the core/baseClient?
Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.
I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?
This PR is missing the sendDefaultPii check!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
8670611 to
fb62082CompareJanuary 15, 2025 08:48
@mydea
mydea
commented
Jan 15, 2025

Copy link


Copy Markdown

MemberAuthor
The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔
Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
522d43a to
b34541aCompareJanuary 15, 2025 14:08
@mydea
mydea
commented
Jan 16, 2025

Copy link


Copy Markdown

MemberAuthor
@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b34541a to
22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea
marked this pull request as ready for review
January 17, 2025 11:15
@mydea
mydea
requested a review
from a team
as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
22b2da1 to
e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich
mentioned this pull request
Jan 17, 2025

Closed

@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
e27c3a3 to
b54f4c2CompareJanuary 17, 2025 13:18

This was referenced Jan 17, 2025

Closed


Closed

timfish
timfish
approved these changes
Jan 17, 2025
View reviewed changes
@timfishtimfish
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
@mydea
mydea
commented
Jan 20, 2025

Copy link


Copy Markdown

MemberAuthor
We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
There is already this issue: #5347
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b54f4c2 to
a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea
merged commit eedf4a5
into
developJan 20, 2025
@mydea
mydea
deleted the

fn/set-user-ip-browser

branch
January 20, 2025 12:28

This was referenced Jan 20, 2025
 Merged

 Merged

@mydeamydea
mentioned this pull request
Jan 20, 2025

Closed

krystofwoldrich added a commit
that referenced
this pull request
Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric

+1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
feat(browser): Set user.ip_address explicitly to {{auto}} - #15008
Merged
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Jan 20, 2025
Merged
feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Conversation
@mydea
@mydeamydea
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Part of getsentry/team-sdks#118
This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.

So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)
This is set for:
  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)
@mydeamydea
self-assigned this
Jan 14, 2025
@mydeamydea
mentioned this pull request
Jan 14, 2025

Open

@github-actions
github-actionsBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺
View base workflow run
@codecov
codecovBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan
Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)
Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@krystofwoldrich
krystofwoldrich
commented
Jan 14, 2025

Copy link


Copy Markdown

Contributor
Why do we implement this only in browser client and not in the core/baseClient?
@timfish
timfish
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Collaborator
Why do we implement this only in browser client and not in the core/baseClient?
Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.
I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?
This PR is missing the sendDefaultPii check!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
8670611 to
fb62082CompareJanuary 15, 2025 08:48
@mydea
mydea
commented
Jan 15, 2025

Copy link


Copy Markdown

MemberAuthor
The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔
Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
522d43a to
b34541aCompareJanuary 15, 2025 14:08
@mydea
mydea
commented
Jan 16, 2025

Copy link


Copy Markdown

MemberAuthor
@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b34541a to
22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea
marked this pull request as ready for review
January 17, 2025 11:15
@mydea
mydea
requested a review
from a team
as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
22b2da1 to
e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich
mentioned this pull request
Jan 17, 2025

Closed

@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
e27c3a3 to
b54f4c2CompareJanuary 17, 2025 13:18

This was referenced Jan 17, 2025

Closed


Closed

timfish
timfish
approved these changes
Jan 17, 2025
View reviewed changes
@timfishtimfish
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
@mydea
mydea
commented
Jan 20, 2025

Copy link


Copy Markdown

MemberAuthor
We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
There is already this issue: #5347
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b54f4c2 to
a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea
merged commit eedf4a5
into
developJan 20, 2025
@mydea
mydea
deleted the

fn/set-user-ip-browser

branch
January 20, 2025 12:28

This was referenced Jan 20, 2025
 Merged

 Merged

@mydeamydea
mentioned this pull request
Jan 20, 2025

Closed

krystofwoldrich added a commit
that referenced
this pull request
Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric

+1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(browser): Set user.ip_address explicitly to {{auto}} - #15008
Merged
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Jan 20, 2025
Merged
feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Conversation
@mydea
@mydeamydea
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Part of getsentry/team-sdks#118
This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.

So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)
This is set for:
  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)
@mydeamydea
self-assigned this
Jan 14, 2025
@mydeamydea
mentioned this pull request
Jan 14, 2025

Open

@github-actions
github-actionsBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺
View base workflow run
@codecov
codecovBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan
Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)
Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@krystofwoldrich
krystofwoldrich
commented
Jan 14, 2025

Copy link


Copy Markdown

Contributor
Why do we implement this only in browser client and not in the core/baseClient?
@timfish
timfish
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Collaborator
Why do we implement this only in browser client and not in the core/baseClient?
Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.
I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?
This PR is missing the sendDefaultPii check!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
8670611 to
fb62082CompareJanuary 15, 2025 08:48
@mydea
mydea
commented
Jan 15, 2025

Copy link


Copy Markdown

MemberAuthor
The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔
Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
522d43a to
b34541aCompareJanuary 15, 2025 14:08
@mydea
mydea
commented
Jan 16, 2025

Copy link


Copy Markdown

MemberAuthor
@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b34541a to
22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea
marked this pull request as ready for review
January 17, 2025 11:15
@mydea
mydea
requested a review
from a team
as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
22b2da1 to
e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich
mentioned this pull request
Jan 17, 2025

Closed

@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
e27c3a3 to
b54f4c2CompareJanuary 17, 2025 13:18

This was referenced Jan 17, 2025

Closed


Closed

timfish
timfish
approved these changes
Jan 17, 2025
View reviewed changes
@timfishtimfish
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
@mydea
mydea
commented
Jan 20, 2025

Copy link


Copy Markdown

MemberAuthor
We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
There is already this issue: #5347
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b54f4c2 to
a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea
merged commit eedf4a5
into
developJan 20, 2025
@mydea
mydea
deleted the

fn/set-user-ip-browser

branch
January 20, 2025 12:28

This was referenced Jan 20, 2025
 Merged

 Merged

@mydeamydea
mentioned this pull request
Jan 20, 2025

Closed

krystofwoldrich added a commit
that referenced
this pull request
Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric

+1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feat(browser): Set user.ip_address explicitly to {{auto}} - #15008
Merged
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Jan 20, 2025
Merged
feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Conversation
@mydea
@mydeamydea
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Part of getsentry/team-sdks#118
This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.

So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)
This is set for:
  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)
@mydeamydea
self-assigned this
Jan 14, 2025
@mydeamydea
mentioned this pull request
Jan 14, 2025

Open

@github-actions
github-actionsBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺
View base workflow run
@codecov
codecovBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan
Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)
Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@krystofwoldrich
krystofwoldrich
commented
Jan 14, 2025

Copy link


Copy Markdown

Contributor
Why do we implement this only in browser client and not in the core/baseClient?
@timfish
timfish
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Collaborator
Why do we implement this only in browser client and not in the core/baseClient?
Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.
I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?
This PR is missing the sendDefaultPii check!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
8670611 to
fb62082CompareJanuary 15, 2025 08:48
@mydea
mydea
commented
Jan 15, 2025

Copy link


Copy Markdown

MemberAuthor
The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔
Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
522d43a to
b34541aCompareJanuary 15, 2025 14:08
@mydea
mydea
commented
Jan 16, 2025

Copy link


Copy Markdown

MemberAuthor
@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b34541a to
22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea
marked this pull request as ready for review
January 17, 2025 11:15
@mydea
mydea
requested a review
from a team
as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
22b2da1 to
e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich
mentioned this pull request
Jan 17, 2025

Closed

@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
e27c3a3 to
b54f4c2CompareJanuary 17, 2025 13:18

This was referenced Jan 17, 2025

Closed


Closed

timfish
timfish
approved these changes
Jan 17, 2025
View reviewed changes
@timfishtimfish
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
@mydea
mydea
commented
Jan 20, 2025

Copy link


Copy Markdown

MemberAuthor
We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
There is already this issue: #5347
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b54f4c2 to
a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea
merged commit eedf4a5
into
developJan 20, 2025
@mydea
mydea
deleted the

fn/set-user-ip-browser

branch
January 20, 2025 12:28

This was referenced Jan 20, 2025
 Merged

 Merged

@mydeamydea
mentioned this pull request
Jan 20, 2025

Closed

krystofwoldrich added a commit
that referenced
this pull request
Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric

+1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
feat(browser): Set user.ip_address explicitly to {{auto}} - #15008
Merged
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Jan 20, 2025
Merged
feat(browser): Set user.ip_address explicitly to {{auto}}#15008
mydea merged 6 commits into
developfrom 
fn/set-user-ip-browser
Conversation
@mydea
@mydeamydea
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Member
Part of getsentry/team-sdks#118
This is so far inferred at relay-side, which is pretty intransparent and potentially confusing.

So instead, we want to explicitly set this in the SDK. If this is set to null it will not be inferred (which is kind of intransparent too, but...)
This is set for:
  • error events (as user.ip_address)
  • transactions (as user.ip_address)
  • replay events (as user.ip_address)
  • profiles (as user.ip_address)
  • sessions (as user.ip_address)
  • session aggregates: as attr.ip_address: '{{auto}}'
  • standalone spans (not by default, but added to all standalone web vitals spans we emit, as 'client.address': '{{auto}}' attribute)
@mydeamydea
self-assigned this
Jan 14, 2025
@mydeamydea
mentioned this pull request
Jan 14, 2025

Open

@github-actions
github-actionsBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Contributor
size-limit report 📦
PathSize% ChangeChange
@sentry/browser22.98 KB+0.51%+119 B 🔺
@sentry/browser - with treeshaking flags21.64 KB+0.46%+101 B 🔺
@sentry/browser (incl. Tracing)35.68 KB+0.33%+120 B 🔺
@sentry/browser (incl. Tracing, Replay)72.48 KB+0.15%+108 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags62.99 KB+0.2%+126 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)76.73 KB+0.13%+101 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)88.74 KB+0.13%+112 B 🔺
@sentry/browser (incl. Feedback)39.2 KB+0.27%+107 B 🔺
@sentry/browser (incl. sendFeedback)27.61 KB+0.38%+105 B 🔺
@sentry/browser (incl. FeedbackAsync)32.37 KB+0.3%+98 B 🔺
@sentry/react25.66 KB+0.36%+94 B 🔺
@sentry/react (incl. Tracing)38.46 KB+0.26%+101 B 🔺
@sentry/vue27.04 KB+0.33%+91 B 🔺
@sentry/vue (incl. Tracing)37.45 KB+0.35%+133 B 🔺
@sentry/svelte23.11 KB+0.48%+111 B 🔺
CDN Bundle24.37 KB+0.37%+91 B 🔺
CDN Bundle (incl. Tracing)36 KB+0.27%+96 B 🔺
CDN Bundle (incl. Tracing, Replay)70.65 KB+0.15%+108 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback)75.79 KB+0.15%+116 B 🔺
CDN Bundle - uncompressed71.17 KB+0.47%+339 B 🔺
CDN Bundle (incl. Tracing) - uncompressed106.82 KB+0.34%+367 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed217.67 KB+0.18%+398 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed230.21 KB+0.17%+398 B 🔺
@sentry/nextjs (client)38.59 KB+0.29%+113 B 🔺
@sentry/sveltekit (client)36.22 KB+0.3%+110 B 🔺
@sentry/node161.37 KB+0.02%+19 B 🔺
@sentry/node - without tracing97.15 KB+0.02%+16 B 🔺
@sentry/aws-serverless127.04 KB+0.01%+13 B 🔺
View base workflow run
@codecov
codecovBot
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

❌ 1 Tests Failed:
Tests completedFailedPassedSkipped
6891688299
View the full list of 1 ❄️ flaky tests
request-instrumentation.test.tsShouldsendatransactionwithafetchspan
Flake rate in main: 40.00% (Passed 6 times, Failed 4 times)
Stack Traces | 30s run time
request-instrumentation.test.ts:4:5Shouldsendatransactionwithafetchspan
To view more test analytics, go to the Test Analytics Dashboard

📢 Thoughts on this report? Let us know!
@krystofwoldrich
krystofwoldrich
commented
Jan 14, 2025

Copy link


Copy Markdown

Contributor
Why do we implement this only in browser client and not in the core/baseClient?
@timfish
timfish
commented
Jan 14, 2025•

edited

Loading

Copy link


Copy Markdown

Collaborator
Why do we implement this only in browser client and not in the core/baseClient?
Yep, good point. I just added this to Electron which would be unnecessary if this was added to core.
I think we don't want this in the Node client (or other server clients) because it would mean all events end up with the server IP address?
This PR is missing the sendDefaultPii check!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
8670611 to
fb62082CompareJanuary 15, 2025 08:48
@mydea
mydea
commented
Jan 15, 2025

Copy link


Copy Markdown

MemberAuthor
The idea behind adding this to browser client directly is because we def. do not want this for server side clients, so IMHO it should not live in core 🤔
Regarding the sendDefaultPii, for now I opted to leave the behavior the same as it was. In a follow up, we can decide if we want to change the default!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
522d43a to
b34541aCompareJanuary 15, 2025 14:08
@mydea
mydea
commented
Jan 16, 2025

Copy link


Copy Markdown

MemberAuthor
@krystofwoldrich & @timfish I ended up rewriting this a bit to use hooks instead of hard-coding this too much into the browser SDK. This means that electron/RN should be able to just copy the hook config from the browser SDK to get the same/consistent behavior!
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b34541a to
22b2da1CompareJanuary 17, 2025 11:13
@mydea
mydea
marked this pull request as ready for review
January 17, 2025 11:15
@mydea
mydea
requested a review
from a team
as a code ownerJanuary 17, 2025 11:15
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
22b2da1 to
e27c3a3CompareJanuary 17, 2025 12:06
@krystofwoldrichkrystofwoldrich
mentioned this pull request
Jan 17, 2025

Closed

@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
e27c3a3 to
b54f4c2CompareJanuary 17, 2025 13:18

This was referenced Jan 17, 2025

Closed


Closed

timfish
timfish
approved these changes
Jan 17, 2025
View reviewed changes
@timfishtimfish
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
@mydea
mydea
commented
Jan 20, 2025

Copy link


Copy Markdown

MemberAuthor
We should open an issue to track sendDefaultPii because currently if you set sendDefaultPii: false it will still result in an IP address being included and it might get forgotten!
There is already this issue: #5347
@mydea
mydeaforce-pushed
the
fn/set-user-ip-browser
branch
from
b54f4c2 to
a8293adCompareJanuary 20, 2025 11:05
@mydea
mydea
merged commit eedf4a5
into
developJan 20, 2025
@mydea
mydea
deleted the

fn/set-user-ip-browser

branch
January 20, 2025 12:28

This was referenced Jan 20, 2025
 Merged

 Merged

@mydeamydea
mentioned this pull request
Jan 20, 2025

Closed

krystofwoldrich added a commit
that referenced
this pull request
Feb 3, 2025
@krystofwoldrich
chore(browser): Export ipAddress helpers for use in other SDKs (#15079)
3c4df06
- Related to #15008
Export helpers to avoid duplications in sentry/react-native.
- RN PR getsentry/sentry-react-native#4466
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@timfishtimfishtimfish approved these changes
@jjbayerjjbayerAwaiting requested review from jjbayer
@cleptriccleptricAwaiting requested review from cleptric

+1 more reviewer

@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydeamydea

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@mydea@krystofwoldrich@timfish@AbhiPrasad