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

scripts/decode-resp: decode flamebearer response for debugging #284

Merged
merged 1 commit into from
Jul 17, 2021
Merged

scripts/decode-resp: decode flamebearer response for debugging #284

merged 1 commit into from
Jul 17, 2021

Conversation

iOliverNguyen
Copy link
Contributor

This PR adds scripts/decode-resp for decoding response.

Decode response from pyroscope, for debugging

Usage: decode-resp -file <response.json>
  where <response.json> is the body response from /render API
  example: http://localhost:4040/render?from=now-1h&until=now&name=pyroscope.server.alloc_objects{}&max-nodes=1024&format=json

Flags:
  -file string
    	path to response file (required)
  -out string
    	name of output file (default to NAME.out.EXT)

]
}
}`
assert.Equal(t, expected, outJson)
Copy link
Member

Choose a reason for hiding this comment

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

Please use ginkgo + gomega for testing stuff, here's an example: https://github.com/pyroscope-io/pyroscope/blob/main/pkg/storage/storage_test.go#L228

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 see. Will change that.

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 updated the test with ginkgo + gomega.

@petethepig
Copy link
Member

This looks good. My only question is why do we need it. Can you explain the usescase?

@iOliverNguyen
Copy link
Contributor Author

As a new contributor, I need to understand the response structure (especially levels with delta encoding), so I created this script:

  • This will help other new contributors when joining the project, to easily decoding/understanding the response (with delta encoding).
  • Also, when we add new data or modify the structure, as discussed in implementing the diff view, this will help to debug.

@codecov
Copy link

codecov bot commented Jul 15, 2021

Codecov Report

Merging #284 (0994fe7) into main (fada901) will decrease coverage by 0.21%.
The diff coverage is 32.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
- Coverage   54.21%   54.01%   -0.20%     
==========================================
  Files          89       91       +2     
  Lines        3638     3687      +49     
==========================================
+ Hits         1972     1991      +19     
- Misses       1466     1496      +30     
  Partials      200      200              
Impacted Files Coverage Δ
scripts/decode-resp/main.go 0.00% <0.00%> (ø)
scripts/decode-resp/decode.go 100.00% <100.00%> (ø)
pkg/agent/session.go 66.40% <0.00%> (+2.46%) ⬆️

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 395477a...0994fe7. Read the comment docs.

@petethepig petethepig self-requested a review July 15, 2021 22:16
@petethepig petethepig merged commit 8c45c81 into grafana:main Jul 17, 2021
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