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

feat: support speedscope format #1589

Merged
merged 9 commits into from
Oct 6, 2022
Merged

Conversation

vasi-stripe
Copy link
Contributor

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:

  • Technically, the times in Speedscope format are allowed to be doubles. It looks like they really are non-integers for pyinstrument! But the tree API only allows uint64 weights, and there's no simple way to decide what resolution the client intends.
  • It's awkward to figure out the right units. The Speedscope format mixes up the concepts of time-units (s/ms/µs) and measurement-units (bytes/allocations/time). I guess at least for time-based units, we base sampleRate on them?
  • It's not obvious to me how we should build keys when there are multiple profiles present of the same type
  • It's not obvious how we should tell when to use metadata from the HTTP params vs. the profile format's metadata. Would be nice if we could distinguish between "HTTP param default" vs. "HTTP client explicitly chose this".
  • It looks like adhoc mode uses a whole different set of converters, so I didn't build that for Speedscope (yet?)
  • Evented mode is often used for flamegraphs, but Pyroscope doesn't really do those, we prefer to aggregate. Seems fine, as long as users expect this.

Closes #172

@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Base: 66.53% // Head: 66.53% // No change to project coverage 👍

Coverage data is based on head (63b83df) compared to base (00ed85a).
Patch has no changes to coverable lines.

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@grafana grafana deleted a comment from CLAassistant Oct 5, 2022
@vasi-stripe
Copy link
Contributor Author

@petethepig I think the npmjs test failure is spurious?

@petethepig
Copy link
Member

@vasi-stripe Yeah, that job shoudn't run for forks. You can ignore it for now. We've also disabled it in this PR

@vasi-stripe
Copy link
Contributor Author

Great! I think the only blockers then are:

  • Dealing with doubles
  • Anything else serious you find on review ;-)

Any thoughts on how to manage non-integer weights? Does Pyroscope assume that weights are in multiples of sampleRate?

@vasi-stripe
Copy link
Contributor Author

Filed an issue on speedscope about confusing units: jlfwong/speedscope#406

@petethepig
Copy link
Member

@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.

Copy link
Member

@petethepig petethepig left a 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 :)

@kolesnikovae
Copy link
Collaborator

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
@vasi-stripe
Copy link
Contributor Author

Ok, merged in the units PR. Thanks for that!

Could you re-run actions?

@petethepig petethepig changed the title Support speedscope format feat: support speedscope format Oct 6, 2022
@petethepig petethepig merged commit edb5e18 into grafana:main Oct 6, 2022
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.

Add support for speedscope format
3 participants