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.

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

Integrates subscribe and fetchAppLogs with developer dashboard #5498

Open
wants to merge 8 commits into
base: ms.add-app-logs-to-devp
Choose a base branch
from
Open

Integrates subscribe and fetchAppLogs with developer dashboard #5498

wants to merge 8 commits into from
+339 −68

Conversation

mssalemi
Copy link
Contributor

@mssalemi mssalemi commented Mar 10, 2025 •
edited
Loading

Requires: #5497
Requires: Partners - js.app-logs-prototype-2Request: Core - 03-07-expose_dev_dash_route_to_manage_app_logs
Part of: https://github.com/Shopify/shopify-functions/issues/605

WHY are these changes introduced?

This PR adds app logs functionality to the AppManagementClient, allowing app logs to be fetched through the Developer Platform client interface.

Key changes:

  • Implemented subscribeToAppLogs and appLogs methods in AppManagementClient
  • Updated the DeveloperPlatformClient interface to include organizationId and appId parameters
    • PartnersClient will ignore them
  • Added new fetchAppLogsDevDashboard in utils
  • Updated all app logs related components to pass through organizationId and appId

WHAT is this pull request doing?

How to test your changes?

All tests have been updated to include the new required parameters. The implementation has been tested with both Partners API and Developer Dashboard API backends.

Currently need different shops / app setup.

With app that lives in partners (I used prod):

pnpm run shopify app logs --path=./path-to-app

pnpm run shopify app dev --path=./path-to-app

Spin Dev Dashboard Setup

USE_APP_MANAGEMENT_API=1 SHOPIFY_SERVICE_ENV=spin pnpm run shopify app dev --path=./spin-dev-dash-app
USE_APP_MANAGEMENT_API=1 SHOPIFY_SERVICE_ENV=spin pnpm run shopify app logs --path=./spin-dev-dash-app

Post-release steps

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

@mssalemi mssalemi changed the title Ms.update amclient with app logs Integrates subscribe and fetchAppLogs with developer dashboard Mar 10, 2025
@mssalemi mssalemi force-pushed the ms.add-app-logs-to-devp branch from 65127cf to 120f6ff Compare March 10, 2025 17:45
@mssalemi mssalemi mentioned this pull request Mar 10, 2025
Draft
5 tasks
@mssalemi mssalemi force-pushed the ms.add-app-logs-to-devp branch from 120f6ff to c645803 Compare March 10, 2025 19:12
@mssalemi mssalemi force-pushed the ms.update-amclient-with-app-logs branch from 0f2fa07 to d916529 Compare March 10, 2025 19:13
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 11, 2025 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
76.19% (-0.12% 🔻)
 9290/12193
🟡 Branches
71.5% (-0.08% 🔻)
 4554/6369
🟡 Functions
75.89% (-0.05% 🔻)
 2418/3186
🟡 Lines
76.74% (-0.12% 🔻)
 8779/11440
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟡
... / utils.ts
66.95% (-8.54% 🔻)
59.65% (-7.02% 🔻)
63.16% (-7.43% 🔻)
66.09% (-8.66% 🔻)
🔴
... / app-management-client.ts
35.51% (-0.29% 🔻)
 27.66% 34.69%
34.51% (-0.29% 🔻)
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
88% (-4% 🔻)
 100%
98.33% (-1.67% 🔻)

Test suite run success

2111 tests passing in 930 suites.

Report generated by 🧪jest coverage report action from e755a53

@mssalemi mssalemi marked this pull request as ready for review March 12, 2025 13:43
@mssalemi mssalemi requested a review from a team as a code owner March 12, 2025 13:43
@github-actions GitHub Actions
Copy link
Contributor

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.

mssalemi
mssalemi commented Mar 12, 2025
View reviewed changes
packages/app/src/cli/utilities/developer-platform-client/app-management-client.ts Outdated
@@ -1067,3 +1104,78 @@ function appModuleVersion(mod: ReleasedAppModuleFragment): Required<AppModuleVer
},
}
}

interface AppLogsSubscribeQueryVariables {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe once the core PR is merged, we can import these types. Leaving here for now.

@mssalemi mssalemi force-pushed the ms.update-amclient-with-app-logs branch from 2c025a3 to e755a53 Compare March 12, 2025 15:29
@mssalemi mssalemi requested review from shauns and jacobsteves March 12, 2025 18:59
@mssalemi mssalemi force-pushed the ms.update-amclient-with-app-logs branch from 3ae5b5b to 54a0f72 Compare March 13, 2025 17:14
jacobsteves
jacobsteves reviewed Mar 13, 2025
View reviewed changes
Copy link
Member

@jacobsteves jacobsteves left a comment

Choose a reason for hiding this comment

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

Looks pretty good! A couple small questions

packages/app/src/cli/services/app-logs/utils.ts Outdated
Comment on lines 154 to 171
const fqdn = await appManagementFqdn()
let url = `https://${fqdn}/app_management/unstable/organizations/${organizationId}/app_logs/poll`

if (!cursor) {
return url
}

url += `?cursor=${cursor}`

if (filters?.status) {
url += `&status=${filters.status}`
}
if (filters?.source) {
url += `&source=${filters.source}`
}

return url
}
Copy link
Member

Choose a reason for hiding this comment

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

I see this is duplicated logic with generateFetchAppLogUrl. What if we created a helper that handled the url params and generateFetchAppLogUrl and generateFetchAppLogUrlDevDashboard can call that function with whatever url they need?

packages/app/src/cli/services/dev/processes/app-logs-polling.ts Outdated
@@ -23,12 +25,16 @@ interface Props {
apiKey: string
}
storeName: string
organizationId: string
appId: string
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need the appId here? I see it was deleted from the subscribe code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! Not needed now, will remove :)

@mssalemi mssalemi force-pushed the ms.add-app-logs-to-devp branch from 49c65b6 to fa530fe Compare March 13, 2025 19:34
@mssalemi mssalemi force-pushed the ms.update-amclient-with-app-logs branch from 54a0f72 to fc01c3f Compare March 13, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobsteves jacobsteves jacobsteves left review comments

@shauns shauns Awaiting requested review from shauns

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mssalemi @jacobsteves