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: ignore root node when converting a flamebearer to a tree. #812

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

abeaumont
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2022

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
webapp/public/assets/app.js 354.13 KB (0%) 7.1 s (0%) 302 ms (+34.18% 🔺) 7.4 s

@pyroscopebot
Copy link
Collaborator

Screenshots

Throughput Throughput
Disk Usage Disk Usage
Memory Memory
Upload Errors (Total) Upload Errors (Total)
Successful Uploads (Total) Successful Uploads (Total)
CPU Utilization CPU Utilization

Result

main pr diff threshold
throughput 203.18 224.25 🟩 21.07 (9.86%) 5%
total items processed 61506.00 66446.00 🟩 4940.00 (7.72%) 5%
Details
Name Description Query for main Query for pr
throughput rate(pyroscope_http_request_duration_seconds_count{handler="/ingest", instance="pyroscope-main:4040"}[5m]) rate(pyroscope_http_request_duration_seconds_count{handler="/ingest", instance="pyroscope:4040"}[5m])
total items processed pyroscope_http_request_duration_seconds_count{handler="/ingest", instance="pyroscope-main:4040"} pyroscope_http_request_duration_seconds_count{handler="/ingest", instance="pyroscope:4040"}

Generated by 🚫 dangerJS against 4f76caf

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #812 (3b5d931) into main (a2e47b2) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #812   +/-   ##
=======================================
  Coverage   73.06%   73.06%           
=======================================
  Files          68       68           
  Lines        2357     2357           
  Branches      430      430           
=======================================
  Hits         1722     1722           
  Misses        607      607           
  Partials       28       28           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbd67f2...3b5d931. Read the comment docs.

@petethepig
Copy link
Member

@abeaumont Can you explain the issue a little bit more?

abeaumont added a commit that referenced this pull request Feb 10, 2022
This will currently fail due to
#812 and
#820
@abeaumont
Copy link
Contributor Author

The problem is related due to the flamebearer format. In a flamebearer, a root node total is added, which serves as the first level. This node doesn't exist in the tree, it's generated in the conversion (see for example https://github.com/pyroscope-io/pyroscope/blob/main/pkg/structs/flamebearer/flamebearer_test.go#L27-L53

When we are doing the reverse conversion, from flamebearer to tree, we need to remove this total node, otherwise it'll be part of the tree, and when the tree is converted back to a flamebearer, it'll have a duplicated total: the one in the tree, and the one added during the tree to flamebearer conversion (if you use the adhoc diff view, you'll see them duplicated there).

I have created a test for the Diff conversion function to showcase the problem: f7ca4a2.
The test is currently failing since that branch doesn't contain the fix: https://github.com/pyroscope-io/pyroscope/runs/5140109084?check_suite_focus=true

(I have also found out a different bug while creating that test, #820)

@abeaumont abeaumont merged commit 7751b15 into main Feb 10, 2022
@abeaumont abeaumont deleted the fix/remove-total-when-importing branch February 10, 2022 11:57
abeaumont added a commit that referenced this pull request Feb 10, 2022
* chore: move the diff generation of two profiles into a function.

This way the function is reusable.

* test: add a test for DiffV1.

This will currently fail due to
#812 and
#820

* test: fix the diff test.

* fix expectation type in test.
korniltsev pushed a commit that referenced this pull request Jul 18, 2023
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.

3 participants