-
Notifications
You must be signed in to change notification settings - Fork 621
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
feat: support speedscope format #1589
Conversation
Codecov ReportBase: 66.53% // Head: 66.53% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #1589 +/- ##
=======================================
Coverage 66.53% 66.53%
=======================================
Files 148 148
Lines 5031 5031
Branches 1165 1165
=======================================
Hits 3347 3347
Misses 1679 1679
Partials 5 5 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@petethepig I think the npmjs test failure is spurious? |
@vasi-stripe Yeah, that job shoudn't run for forks. You can ignore it for now. We've also disabled it in this PR |
Great! I think the only blockers then are:
Any thoughts on how to manage non-integer weights? Does Pyroscope assume that weights are in multiples of sampleRate? |
Filed an issue on speedscope about confusing units: jlfwong/speedscope#406 |
@vasi-stripe It's looking good! Let me think a little more about the double thing and time units / sample rate. I'll review tonight. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks and works great — I tried a few speedscope jsons I found online and they were getting ingested without any issues.
The only things I noticed that I figured we should improve before merging are:
- as @vasi-stripe mentioned, dealing with
double
values (we only support integers in the storage engine) - dealing with time units such as seconds or milliseconds (most of our integrations deal with samples, which are then combined with
sampleRate
to render time units, e.g seconds).pyinstrument
uses seconds and without sample rate adjustments the numbers on the frontend don't match values in speedscope.
I figured this is getting deep into the intricacies of our storage engine, so I put together a PR that addresses these. @vasi-stripe I can't commit this to your repo, but I think you should be able to merge that PR in your repo and it will become a part of this PR in pyroscope-io/pyroscope
.
@vasi-stripe & @kolesnikovae — would love your all's feedback on it and if it's looking good let's merge it and do a release soon :)
Looks really good. Nice job @vasi-stripe! @petethepig I left a comment in the patch PR as it mostly concerns the storage internals and the ingestion path on the whole |
adds time units handling for speedscope format
Ok, merged in the units PR. Thanks for that! Could you re-run actions? |
This supports both variants (evented and sampled) of Speedscope's native format. This would allow supporting more agents, eg pyinstrument.
Ingestion using the API appears to work with some sample profiles I have lying arounds. I added a couple of fixture tests from upstream.
Possible issues:
Closes #172