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] Add changes from Cloud Profiles UI to OSS docs #3243

Merged

Conversation

knylander-grafana
Copy link
Contributor

Add doc changes from the Cloud Profiles UI docs back to the Pyroscope OSS equivalent. Based on https://github.com/grafana/website/pull/19509

@knylander-grafana knylander-grafana added the type/docs Improvements for doc docs. Used by Docs team for project management label Apr 23, 2024
@knylander-grafana knylander-grafana self-assigned this Apr 23, 2024
@knylander-grafana knylander-grafana requested review from a team as code owners April 23, 2024 22:03
@knylander-grafana knylander-grafana changed the title [DOC] Add changes from Cloud Profiles page to OSS docs [DOC] Add changes from Cloud Profiles UI to OSS docs Apr 23, 2024
@@ -74,7 +74,7 @@ The Single View page in Pyroscope's UI is built for in-depth profile analysis. H
<!-- Visual Placeholder:** *Screenshots demonstrating each view option in the Single View page.* -->

Let's say that your app has a spike in memory usage.
Without profiling, you would go from a memory spike to digging through code or guessing the cause.
Without profiling, you would go from a spike CPU usage metric to digging through code or guessing the cause.
Copy link
Contributor

@aleks-p aleks-p Apr 25, 2024

Choose a reason for hiding this comment

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

This seems off, as we talk about memory in the previous line. Do we want to reuse the entire paragraph from https://github.com/grafana/website/pull/19509/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the entire text from the website PR:

This screenshot shows a spike in CPU usage.
Without profiling, you would go from a spike in CPU usage metric to digging through code or guessing what the cause.
However, with profiling, you can use the flame graph and table to see exactly which function is most responsible for the spike.
Often this shows up as a single node taking up a noticeably disproportionate width in the flame graph as seen below with the checkDriverAvailability function.

From what you're saying, it looks like we should replace "Let's say that your app has a spike in memory usage" with "This screenshot shows a spike in CPU usage."

Is there additional text you would like to see included?

Copy link
Contributor

@aleks-p aleks-p Apr 25, 2024

Choose a reason for hiding this comment

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

Yes, that replacement makes sense since the screenshot we show after this text (https://grafana.com/docs/pyroscope/latest/view-and-analyze-profile-data/pyroscope-ui/#single-view) has a CPU profile.

Is there additional text you would like to see included?

Not really, the "fix" above should be enough. I do however think we have a minor phrasing issue in this line (also present in https://github.com/grafana/website/pull/19509)

Here we say

Without profiling, you would go from a spike CPU usage metric to digging through code or guessing the cause.

In https://github.com/grafana/website/pull/19509 we say

Without profiling, you would go from a spike in CPU usage metric to digging through code or guessing what the cause.

This could work:

Without profiling, you would go from the CPU usage metric spike to digging through code or guessing the cause.

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 think the bottom sentence is what it's supposed to be. The original PR I missed deleting "what".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@aleks-p aleks-p left a comment

Choose a reason for hiding this comment

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

I found a small phrasing issue, otherwise it looks good!

@knylander-grafana knylander-grafana merged commit bf51cd8 into grafana:main Apr 26, 2024
16 checks passed
@knylander-grafana knylander-grafana added the backport release/v1.5 This label will backport a merged PR to the release/v1.5 branch label Apr 26, 2024
github-actions bot pushed a commit that referenced this pull request Apr 26, 2024
* Add changes from Cloud Profiles page to OSS docs

* Apply suggestions from code review

* Apply suggestions from code review

(cherry picked from commit bf51cd8)
Copy link
Contributor

The backport to release/v1.5 failed:

Validation Failed: "Could not resolve to a node with the global id of 'T_kwDOAG3Mbc4AczmP'."

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-3243-to-release/v1.5 origin/release/v1.5
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x bf51cd8484839f3fda82c3ce75b4bc1f78fead68

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-3243-to-release/v1.5
# Create the PR body template
PR_BODY=$(gh pr view 3243 --json body --template 'Backport bf51cd8484839f3fda82c3ce75b4bc1f78fead68 from #3243{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "[release/v1.5] [DOC] Add changes from Cloud Profiles UI to OSS docs" --body-file - --label "type/docs" --label "backport" --base release/v1.5 --milestone release/v1.5 --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-3243-to-release/v1.5

# Create a pull request where the `base` branch is `release/v1.5` and the `compare`/`head` branch is `backport-3243-to-release/v1.5`.

# Remove the local backport branch
git switch main
git branch -D backport-3243-to-release/v1.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release/v1.5 This label will backport a merged PR to the release/v1.5 branch backport-failed type/docs Improvements for doc docs. Used by Docs team for project management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants