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

fix(serverstats): use telemetry hook in component COMPASS-8056 #5988

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

Anemy
Copy link
Member

@Anemy Anemy commented Jul 1, 2024

COMPASS-8056

Looks like a bug that would be caught by typescript so I also updated the file to typescript. Happy to instead keep it js and make the one line change of using track instead of logger from the component props.
We should refactor all of this performance page, so I refrained from doing more than just js -> ts for now. I did remove the error display though as it wasn't ever used.

Comment on lines 29 to 35
const onShow = useCallback((newDataToShow: CurrentOpData) => {
setData(newDataToShow);
}, []);

const onHide = useCallback(() => {
setData(null);
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nit: because .listen method returns unsubscribe functions, you dont even need extra useCallbacks here, can just define those functions right in the listen methods, less code and runtime

Copy link
Collaborator

@mcasimir mcasimir left a comment

Choose a reason for hiding this comment

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

@Anemy @paula-stacho is this related with #5977, do we need to send a connection_id?

@paula-stacho
Copy link
Contributor

paula-stacho commented Jul 2, 2024

@Anemy @paula-stacho is this related with #5977, do we need to send a connection_id?

yes, done this in https://github.com/mongodb-js/compass/pull/5977/files#diff-2eac96a4c76cb2ac0fa28e8b3e11457b5e7f08871b257ad724ea32e427cb77cbR47-R51 and also noticed this logger.track that slipped under the radar 🙈 . I haven't done the TS update. happy to get this merged first as it fixes the event and then I can update my PR

Copy link
Contributor

@paula-stacho paula-stacho left a comment

Choose a reason for hiding this comment

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

thanks for noticing Rhys!

@Anemy Anemy merged commit c2490fe into main Jul 2, 2024
27 of 28 checks passed
@Anemy Anemy deleted the COMPASS-8056 branch July 2, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants