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.

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

feat(webapp): add tracing page #1433

Merged
merged 59 commits into from
Sep 13, 2022
Merged

feat(webapp): add tracing page #1433

merged 59 commits into from
Sep 13, 2022

Conversation

dogfrogfog
Copy link
Contributor

@dogfrogfog dogfrogfog commented Aug 22, 2022

Brief

  • docs.google

Changes

  • add /exemplars/single page
  • rename /tracing -> /exemplars/merge
  • add heatmap component
  • add selection to heatmap compoent
  • fetch flamegraph with heatmap selected area values

Concerns

  • resize bug

@dogfrogfog dogfrogfog changed the title feat: add tracing page feat(webapp): add tracing page Aug 22, 2022
@CLAassistant
Copy link

CLAassistant commented Aug 23, 2022

CLA assistant check
All committers have signed the CLA.

* add temporary json to display heatmap
* add heatmap settings and display temporary json as a heatmap
* fix heatmap axis styles and active cell styles
* document the series input data format
* print the clicked item coordinates (x, y)
@dogfrogfog dogfrogfog force-pushed the feature/add-tracing-page branch from 83b36bb to fc82933 Compare August 23, 2022 12:54
@dogfrogfog dogfrogfog force-pushed the feature/add-tracing-page branch from 5bc7c76 to 1d3c7df Compare August 23, 2022 13:17
@codecov
Copy link

codecov bot commented Aug 23, 2022

Codecov Report

Merging #1433 (cbbe772) into main (f56fbb0) will decrease coverage by 2.32%.
The diff coverage is 44.94%.

@@            Coverage Diff             @@
##             main    #1433      +/-   ##
==========================================
- Coverage   68.61%   66.29%   -2.31%     
==========================================
  Files         132      139       +7     
  Lines        4360     4752     +392     
  Branches     1179     1288     +109     
==========================================
+ Hits         2991     3150     +159     
- Misses       1363     1597     +234     
+ Partials        6        5       -1     
Impacted Files Coverage Δ
webapp/javascript/services/render.ts 17.18% <9.68%> (-2.27%) ⬇️
webapp/javascript/redux/reducers/tracing.ts 20.62% <20.00%> (ø)
...ipt/components/Heatmap/useHeatmapSelection.hook.ts 32.96% <32.96%> (ø)
webapp/javascript/components/Heatmap/utils.ts 35.49% <35.49%> (ø)
.../javascript/components/Heatmap/Heatmap.module.scss 61.54% <61.54%> (ø)
webapp/javascript/components/Heatmap/index.tsx 74.72% <74.72%> (ø)
webapp/javascript/components/Heatmap/constants.ts 100.00% <100.00%> (ø)
webapp/javascript/components/Sidebar.tsx 86.82% <100.00%> (+1.42%) ⬆️
webapp/javascript/pages/constants.ts 100.00% <100.00%> (ø)
webapp/javascript/services/exemplarsTestData.ts 100.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 428.76 KB (+1.71% 🔺) 8.6 s (+1.71% 🔺) 3.2 s (-1.74% 🔽) 11.7 s
webapp/public/assets/app.css 16.28 KB (+2.56% 🔺) 326 ms (+2.56% 🔺) 0 ms (+100% 🔺) 326 ms
webapp/public/assets/styles.css 9.49 KB (0%) 190 ms (0%) 0 ms (+100% 🔺) 190 ms
packages/pyroscope-flamegraph/dist/index.js 92.08 KB (0%) 1.9 s (0%) 1.1 s (-25.98% 🔽) 2.9 s
packages/pyroscope-flamegraph/dist/index.node.js 91.97 KB (0%) 1.9 s (0%) 554 ms (+35.8% 🔺) 2.4 s
packages/pyroscope-flamegraph/dist/index.css 7.32 KB (0%) 147 ms (0%) 0 ms (+100% 🔺) 147 ms

@eh-am
Copy link
Collaborator

eh-am commented Aug 23, 2022

/create-server

@dogfrogfog
Copy link
Contributor Author

Monosnap.screencast.2022-08-26.19-20-47.mp4

@Rperry2174
Copy link
Contributor

/create-server

* add area selection
* add click selection
* add bucket items colors
* add resizable selected area
@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog
Copy link
Contributor Author

/create-server

@dogfrogfog dogfrogfog marked this pull request as ready for review September 12, 2022 11:43
@Rperry2174
Copy link
Contributor

/create-server

@@ -70,6 +70,7 @@ interface mergeWithQueryIDProps {
maxNodes: string | number;
}

// z.infer<typeof MergeMetadataSchema> ?
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need this here

signal?: AbortSignal;
}
): Promise<Result<HeatmapOutput, RequestError | ZodError>> {
// todo use common builder
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're going to leave this comment here can you elaborate on what you mean by this?

const [minTextEl, maxTextEl] = within(
screen.getByTestId('color-scale')
).getAllByRole('textbox');
// expect(minTextEl.textContent).toBe(`${exemplarsQueryHeatmap.minDepth - 1}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these here?

}),
}));

// TODO(dogfrogfog): refactor
Copy link
Contributor

@Rperry2174 Rperry2174 Sep 12, 2022

Choose a reason for hiding this comment

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

refactor in what way?

Copy link
Collaborator

@eh-am eh-am left a comment

Choose a reason for hiding this comment

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

Wow, Looks good and quite impressive!!

I would like to see the boundaries more clear though:
A generic Heatmap that you pass data and callback events. Right now it's too intertwined with tracing stuff. As an illustration, the HeatMap component should know nothing about the redux store or its state. It should just receive heatmapData (in whatever format it has) and its callbacks (onSelection I guess?).

}));

// TODO(dogfrogfog): refactor
describe.skip('Component: Heatmap', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't be skipped

(acc, [key, value]) => acc + (acc ? `&${key}=${value}` : `${key}=${value}`),
''
);
const response = await request(`/api/exemplars:query?${queryString}`, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kolesnikovae I am curious about the usage of : in a url?

webapp/javascript/services/render.ts Outdated Show resolved Hide resolved
state.heatmapSingleView = {
...action.payload,
// refactor types
heatmap: state.heatmapSingleView.heatmap as Heatmap,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this assertion needed?

webapp/javascript/redux/reducers/tracing.ts Outdated Show resolved Hide resolved
webapp/javascript/redux/reducers/tracing.ts Outdated Show resolved Hide resolved
Comment on lines 218 to 231
useEffect(() => {
if (from && until && query) {
const fetchData = dispatch(
fetchHeatmapSingleView({
query,
from,
until,
...DEFAULT_HEATMAP_PARAMS,
})
);
return () => fetchData.abort('cancel');
}
return undefined;
}, [from, until, query]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is waaaay too much functionality for this single hook. Let's move data fetching to the component/another hook.

dogfrogfog and others added 7 commits September 13, 2022 07:50
Co-authored-by: eduardo aleixo <eh-am@users.noreply.github.com>
Co-authored-by: eduardo aleixo <eh-am@users.noreply.github.com>
* remove comments
* use URLSearchParams to build query params
* refactor tracing state
* tests
* add onSelect handler to Heatmap props
@dogfrogfog
Copy link
Contributor Author

/create-server

@Rperry2174
Copy link
Contributor

/create-server

Copy link
Contributor

@Rperry2174 Rperry2174 left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @dogfrogfog

@Rperry2174 Rperry2174 merged commit 587379a into main Sep 13, 2022
@Rperry2174 Rperry2174 deleted the feature/add-tracing-page branch September 13, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants