Skip to content

Set the user ID for Observe #6458

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.

Sign up for GitHub

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

Jump to bottom
Merged
gonzaloriestra merged 4 commits into from
Sep 30, 2025
Merged

Set the user ID for Observe #6458

gonzaloriestra merged 4 commits into from
Sep 30, 2025

Conversation

@gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Sep 30, 2025 •
edited
Loading

WHY are these changes introduced?

We are not setting the user ID for Observe, so it's using the IP as a fallback:

userId

More info: https://vault.shopify.io/page/Error-Management-FAQ~NUCh.md#how-are-unique-users-calculated

WHAT is this pull request doing?

Properly set the user ID if present

30-19-04cu5-3auz4

How to test your changes?

  • npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]
  • shopify wronggg
  • Look for your error on Observe and check the __observe_session_stability_user field

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@gonzaloriestra
Copy link
Contributor Author

gonzaloriestra commented Sep 30, 2025

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Sep 30, 2025

🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]

Caution

After installing, validate the version by running just shopify in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 30, 2025 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
78.77% (+0.03% 🔼)
 13727/17427
🟡 Branches
72.48% (+0.02% 🔼)
 6688/9228
🟡 Functions
78.8% (+0.02% 🔼)
 3524/4472
🟡 Lines
79.1% (+0.02% 🔼)
 12976/16404
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / conf-store.ts
 100%
82.22% (-2.22% 🔻)
 100% 100%
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
92% (-4% 🔻)
 100%
98.33% (-1.67% 🔻)
🔴
... / environment.ts
40% (-5% 🔻)
 41.18%
50% (-10% 🔻)
42.11% (-5.26% 🔻)

Test suite run success

3338 tests passing in 1383 suites.

Report generated by 🧪jest coverage report action from a492300

@gonzaloriestra gonzaloriestra marked this pull request as ready for review September 30, 2025 11:23
@gonzaloriestra gonzaloriestra requested a review from a team as a code owner September 30, 2025 11:23
@github-actions
Copy link
Contributor

github-actions bot commented Sep 30, 2025

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

MitchDickinson
MitchDickinson approved these changes Sep 30, 2025
View reviewed changes
Copy link
Contributor

@MitchDickinson MitchDickinson left a comment

Choose a reason for hiding this comment

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

Can we add a test before shipping as well?

gonzaloriestra
gonzaloriestra commented Sep 30, 2025
View reviewed changes
packages/cli-kit/src/public/node/error-handler.ts

if (report) {
initializeBugsnag()
const userId = await getLastSeenUserIdAfterAuth()
Copy link
Contributor Author

@gonzaloriestra gonzaloriestra Sep 30, 2025

Choose a reason for hiding this comment

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

This is returning unknown when not found. For example, when running shopify version unauthenticated.

@MitchDickinson should we better send undefined in that case so Observe sets the IP in that case? Not sure what's best...

Copy link
Contributor

@MitchDickinson MitchDickinson Sep 30, 2025

Choose a reason for hiding this comment

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

Falling back to the IP if the user ID is not found makes sense.

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Sep 30, 2025
Merged via the queue into main with commit 34a7758 Sep 30, 2025
2 checks passed
@gonzaloriestra gonzaloriestra deleted the fix-observe-user branch September 30, 2025 14:50
@gonzaloriestra gonzaloriestra restored the fix-observe-user branch September 30, 2025 14:55
gonzaloriestra added a commit that referenced this pull request Sep 30, 2025
@gonzaloriestra
Merge pull request #6458 from Shopify/fix-observe-user
5cf22fa 
Set the user ID for Observe
@gonzaloriestra gonzaloriestra mentioned this pull request Sep 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MitchDickinson MitchDickinson MitchDickinson approved these changes

@plvaldes plvaldes plvaldes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gonzaloriestra @MitchDickinson @plvaldes