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

doc: revamp gc trace documentation #572

Merged
merged 10 commits into from
Aug 18, 2022
Merged

doc: revamp gc trace documentation #572

merged 10 commits into from
Aug 18, 2022

Conversation

tony-go
Copy link
Member

@tony-go tony-go commented Jul 18, 2022

Hi Diag Fam 👋

Context

I recently wrote down a guide and pushed a PR on the nodejs.org repo.

As discussed in the recent meeting, it was probably not the best approach as we already have a guide in this repo. Also, the main strategy was to write documentation here first, then move it to the nodejs.org repo as a guide.

Update

EDIT: I finally just keep exercises here. The content is available in this PR: nodejs/nodejs.org#4775

@tony-go tony-go self-assigned this Jul 18, 2022
@tony-go tony-go changed the title doc: revamp gc_trace documentation doc: revamp gc trace documentation Jul 18, 2022
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

Amazing! I'd just move the changes in using_gc_traces.md to a separate document. Since we have merged that documentation to nodejs.org, we need to remove it from here.

documentation/memory/step3/using_gc_traces.md Outdated Show resolved Hide resolved
@tony-go tony-go requested a review from RafaelGSS August 7, 2022 16:33
documentation/memory/step3/exercice/server.js Outdated Show resolved Hide resolved
documentation/memory/step3/using_gc_traces.md Outdated Show resolved Hide resolved
documentation/memory/step3/using_gc_traces.md Outdated Show resolved Hide resolved
documentation/memory/step3/using_gc_traces.md Outdated Show resolved Hide resolved
documentation/memory/step3/using_gc_traces.md Outdated Show resolved Hide resolved
documentation/memory/step3/using_gc_traces.md Outdated Show resolved Hide resolved
documentation/memory/step3/using_gc_traces.md Outdated Show resolved Hide resolved
@tony-go
Copy link
Member Author

tony-go commented Aug 8, 2022

Thanks for all your suggestions @RafaelGSS 🥰

@tony-go tony-go requested a review from RafaelGSS August 8, 2022 16:27
@tony-go
Copy link
Member Author

tony-go commented Aug 12, 2022

Hey @RafaelGSS 👋 I just added a script idea (alternative). Could you please share your thoughts on it?

Note: it's just a script, I didn't update the tutorial atm.

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

Sorry to be very strict in this PR 😅

documentation/memory/step3/exercice/script.js Outdated Show resolved Hide resolved
documentation/memory/step3/exercice/script.js Outdated Show resolved Hide resolved
documentation/memory/step3/exercice/script.js Outdated Show resolved Hide resolved
documentation/memory/step3/exercice/script.js Outdated Show resolved Hide resolved
@tony-go
Copy link
Member Author

tony-go commented Aug 16, 2022

I integrated the new script into the tutorial @RafaelGSS

Once you'll finish the review. Should I raise a PR on the node repo?

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

I'd just move the changes in using_gc_traces.md to a separate document

Well, I still support this opinion... but, I won't block it anymore.

cc: @nodejs/diagnostics


I believe there is a typo in exercice, that should be exerciSe.


Should I raise a PR on the node repo?

Yes, please

@tony-go
Copy link
Member Author

tony-go commented Aug 16, 2022

I'd just move the changes in using_gc_traces.md to a separate document

Maybe I didn't get the purpose properly: "Since we have merged that documentation to nodejs.org, we need to remove it from here." - Could you elaborate a bit?

I mean, I'll delete the file just before the merge and just let the exercise if it was your point. Or maybe It's for readability (during the review)?

@RafaelGSS
Copy link
Member

My idea is:

  • Create an README.md inside exercise folder
  • The file will contain all the steps to perform the exercise
  • So, instead of modifying all the existing content inside using_gc_traces.md, you could just include something like:
+ The Diagnostics WG created an exercise that you can practice and improve your gc-tracing skills, check it _here_ (link to the exercise folder)

This way we'll have two kinds of documents, the first one a pure and raw tutorial (using_gc_traces.md) and the second one for beginners (exercise/README.md). Does that make sense to you?

@tony-go
Copy link
Member Author

tony-go commented Aug 16, 2022

This way we'll have two kinds of documents, the first one a pure and raw tutorial (using_gc_traces.md) and the second one for beginners (exercise/README.md). Does that make sense to you?

I understand your point but don't understand why the guide should stay pure and raw. When I look at other guides, they seem guided and detailed. Even If I understand the purpose of "pure and raw" I think that is more suited to documentation rather than guides.

Anyway, I could adopt a different approach if we decide to follow that way.

Maybe the team could share his view on this cc @nodejs/diagnostics

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

That's fair enough

So, my final review is just:

  • Fix the typo exercise
  • Perhaps include the ## Examples of diagnosing memory issues with trace option:

Great work!


For more information, you can refer to [the documentation about performance hooks](https://nodejs.org/api/perf_hooks.html).

## Examples of diagnosing memory issues with trace option:
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we use it somehow? The content is pretty good.

Copy link
Member Author

Choose a reason for hiding this comment

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

I use them in the body of the guide.

@RafaelGSS
Copy link
Member

Please remember to open your changes in the nodejs.org project. The using_gc_traces.md file has been removed from this repository.

@tony-go
Copy link
Member Author

tony-go commented Aug 17, 2022

Seems to be ready @RafaelGSS

Maybe should we add a README in the exercice folder ?

@RafaelGSS
Copy link
Member

Seems to be ready @RafaelGSS

Maybe should we add a README in the exercice folder ?

Yes, please

@tony-go tony-go requested a review from RafaelGSS August 17, 2022 21:15
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@RafaelGSS RafaelGSS merged commit 9b020ad into main Aug 18, 2022
@RafaelGSS RafaelGSS deleted the doc/trace-gc branch August 18, 2022 01:09
@tony-go
Copy link
Member Author

tony-go commented Aug 18, 2022

🎉

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.

2 participants