-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore(preprod): amplitude events for emerge pages (EME-638) #103801
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
Conversation
static/app/views/preprod/buildComparison/main/sizeCompareSelectedBuilds.tsx
Outdated
Show resolved
Hide resolved
| pageLinks={pageLinks} | ||
| organizationSlug={organization.slug} | ||
| projectSlug={projectSlug} | ||
| onRowClick={handleBuildRowClick} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: hasSearchQuery prop always passes true instead of variable
The hasSearchQuery prop is written without a value, which in JSX always evaluates to true. This ignores the hasSearchQuery variable defined earlier that contains the actual search query state. The prop should be hasSearchQuery={hasSearchQuery} to pass the variable's value. This causes the table to always behave as if there's a search query, even when there isn't one.
| <LinkButton | ||
| to={buildUrl} | ||
| onClick={() => | ||
| trackPreprodBuildAnalytics('preprod.builds.compare.go_to_build_details', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually we avoid creating these specific track analytics functions if possible - if you search for trackAnalytics, you'll see that this generic function can work for analytics from any team
static/app/views/preprod/buildComparison/main/sizeCompareSelectedBuilds.tsx
Show resolved
Hide resolved
static/app/views/releases/detail/commitsAndFiles/preprodBuilds.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| const handleConfirmDelete = () => { | ||
| handleDeleteArtifact(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we want analytics here?
| organization, | ||
| build_id: build.id, | ||
| project_slug: projectId, | ||
| project_type: headBuildDetails.app_info?.platform ?? null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are both project_type and platform needed when they are both being set to a platform value?
| const organization = useOrganization(); | ||
| const {projectId} = useParams<{projectId: string}>(); | ||
| const platform = headBuildDetails.app_info?.platform ?? null; | ||
| const projectType = headBuildDetails.app_info?.platform ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, is projectType == platform intentional? There's probably a context to fetch the current project we should be using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah projectType is supposed to be the project.type, which is different from the platform, but we don't have that data available in the frontend currently. I'm going to drop it where its not available, and put up a linear ticket to eventually follow with that info. For the time being, it is reconstructible with project_slug + org so if we need to analyze it we can
| platform: buildDetailsData.app_info?.platform ?? null, | ||
| build_id: buildDetailsData.id, | ||
| project_slug: projectId, | ||
| project_type: buildDetailsData.app_info?.platform ?? null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Inconsistent project_type usage in analytics events
The project_type parameter is being set to platform values in two analytics events, contradicting the PR discussion where the maintainer stated they would drop project_type where it's not available. All other analytics events in this PR correctly omit project_type, but preprod.builds.details.delete_build and preprod.builds.release.build_row_clicked incorrectly set project_type to platform values instead of omitting it.
Additional Locations (1)
static/app/views/preprod/buildComparison/main/sizeCompareSelectionContent.tsx
Outdated
Show resolved
Hide resolved
static/app/views/preprod/buildDetails/header/buildDetailsHeaderContent.tsx
Show resolved
Hide resolved
| platform: platformProp ?? null, | ||
| project_type: projectType, | ||
| source: 'metric_card', | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing analytics parameters in build details events
Analytics events for preprod.builds.details.open_insights_sidebar, preprod.builds.details.expand_insight, and preprod.builds.details.open_insight_details_modal are missing build_id and project_slug parameters. These components don't receive or retrieve the build ID and project slug that are available in parent components, leading to incomplete analytics tracking. Other analytics events in the same PR (like in buildDetailsHeaderContent.tsx) include these parameters, showing the inconsistency.
add amplitude events for size analysis pages
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.