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.

Sign up for GitHub

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

Jump to bottom

Profiling: Enable flame graph & Phlare/Parca data sources for all users #63488

Merged
merged 4 commits into from
Mar 1, 2023
Merged

Profiling: Enable flame graph & Phlare/Parca data sources for all users #63488

merged 4 commits into from
Mar 1, 2023

Conversation

joey-grafana
Copy link
Contributor

@joey-grafana joey-grafana commented Feb 21, 2023 •
edited
Loading

What is this feature?

This PR removes the flame graph feature toggle.

Why do we need this feature?

The toggle is no longer needed.

Who is this feature for?

Users of Phlare/Parca/Flame graph.

Special notes for your reviewer:

Also removes the beta badge from the flame graph panel in dashboards.

@joey-grafana joey-grafana added this to the 9.5.0 milestone Feb 21, 2023
@joey-grafana joey-grafana self-assigned this Feb 21, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 21, 2023 •
edited
Loading

⚠️   Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-data has possible breaking changes (more info)

Console output
Read our guideline

@grafanabot grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Feb 21, 2023
@github-actions
Copy link
Contributor

Backend code coverage report for PR #63488
No changes

@github-actions
Copy link
Contributor

Frontend code coverage report for PR #63488

Plugin Main PR Difference
explore 86.26% 86.32% .06%

@joey-grafana joey-grafana marked this pull request as ready for review February 21, 2023 14:21
@joey-grafana joey-grafana requested review from ashharrison90, yaelleC, andresmgot, papagian, zserge and mildwonkey and removed request for a team February 21, 2023 14:21
@joey-grafana
Copy link
Contributor Author

I'd imagine it's ok that levitate is detecting a breaking change (as we're removing a feature toggle)

Screenshot 2023-02-21 at 14 23 27

@ifrost
Copy link
Contributor

ifrost commented Feb 22, 2023

Out of curiosity, why is the flag being removed instead of rolling it out gradually to Cloud?

@ifrost
Copy link
Contributor

ifrost commented Feb 23, 2023 •
edited
Loading

This will be added to the changelog. Should it also be included in the "what's new?" (there's a separate label for it)? I think changing the title to something more meaningful for the changelog would be good.

ifrost
ifrost approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@ifrost ifrost left a comment

Choose a reason for hiding this comment

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

Explore changes LGTM 🎉

I've noticed that suggested visualizations don't list Flame graph, it'd be nice to have it there:

Screenshot 2023-02-23 at 11 47 02

aocenas
aocenas approved these changes Feb 23, 2023
View reviewed changes
Copy link
Member

@aocenas aocenas left a comment

Choose a reason for hiding this comment

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

Looks good

@aocenas
Copy link
Member

aocenas commented Feb 23, 2023

@ifrost

why is the flag being removed instead of rolling it out gradually to Cloud?

Do you think there is a specific benefit to that? Considering this is already tested and it's a new functionality (less prone to cause errors in existing ones) I feel like rolling it slowly on the cloud does not give us much and will still mean OSS users would need to separately enable the flag.

@L-M-K-B
Copy link
Contributor

L-M-K-B commented Feb 23, 2023

You might want to consider changing the headline of the PR as it's going to be added to the changelog. What about something like Profiling: Enable flame graph for all users ?

chri2547
chri2547 approved these changes Feb 23, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

Docs lgtm! Thank you for making these changes!

@joey-grafana joey-grafana changed the title Profiling: Remove flame graph feature toggle Profiling: Enable flame graph for all users Feb 24, 2023
@joey-grafana
Copy link
Contributor Author

Updated the PR title and added the add to what's new label :)

Suggested visualizations is a nice tip, thanks. I've made a note to check that out.

@joey-grafana joey-grafana changed the title Profiling: Enable flame graph for all users Profiling: Enable flame graph & Phlare/Parca data sources for all users Feb 24, 2023
@joey-grafana joey-grafana merged commit fbd049a into main Mar 1, 2023
@joey-grafana joey-grafana deleted the joey/remove-flame-graph-toggle branch March 1, 2023 11:32
ryantxu pushed a commit that referenced this pull request Mar 2, 2023
@joey-grafana @ryantxu
Profiling: Enable flame graph & Phlare/Parca data sources for all use…
41d8aa8 
…rs (#63488)

* Remove flame graph toggle

* Remove beta badge from panel

* Update expectedListResp.json

* Update flame graph container to only show if there is data
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ifrost ifrost ifrost approved these changes

@aocenas aocenas aocenas approved these changes

@chri2547 chri2547 chri2547 approved these changes

@Eve832 Eve832 Awaiting requested review from Eve832

@GrafanaWriter GrafanaWriter Awaiting requested review from GrafanaWriter

@ashharrison90 ashharrison90 Awaiting requested review from ashharrison90 ashharrison90 was automatically assigned from grafana/grafana-frontend-platform

@yaelleC yaelleC Awaiting requested review from yaelleC yaelleC was automatically assigned from grafana/grafana-frontend-platform

@andresmgot andresmgot Awaiting requested review from andresmgot andresmgot was automatically assigned from grafana/plugins-platform-frontend

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@mildwonkey mildwonkey Awaiting requested review from mildwonkey mildwonkey was automatically assigned from grafana/backend-platform

Assignees

@joey-grafana joey-grafana

Labels
add to changelog add to what's new area/backend area/datasource/frontend area/explore area/frontend area/panel/flame-graph area/profiling datasource/Parca Parca datasource datasource/Phlare Phlare datasource levitate breaking change A label indicating a breaking change and assigned by Levitate. no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
9.5.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@joey-grafana @ifrost @aocenas @L-M-K-B @chri2547 @grafanabot