-
Notifications
You must be signed in to change notification settings - Fork 714
feat: add support for labels when used with JFR and async-profiler's contextId #1096
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
Conversation
size-limit report 📦
|
Codecov Report
@@ Coverage Diff @@
## main #1096 +/- ##
==========================================
+ Coverage 70.79% 70.80% +0.02%
==========================================
Files 93 94 +1
Lines 3022 3047 +25
Branches 757 759 +2
==========================================
+ Hits 2139 2157 +18
- Misses 879 886 +7
Partials 4 4
Continue to review full report at Codecov.
|
petethepig
left a comment
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.
It looks good overall. There are a few go nits, but I think performance-wise and code-wise it all looks good.
A couple of things we need to change before merging this:
- parse http form in a more reliable manner (see comments above)
- add some tests here — i think a simple new test ensuring that multiple tags get created when you upload a jfr with tags should be good enough.
- fix linting errors
- address other comments above
- rename tags to labels
pkg/parser/parser.go
Outdated
|
|
||
| func parseJFRMultipart(ctx context.Context, in *PutInput, err error, p *Parser, pi *storage.PutInput) error { | ||
| reader := multipart.NewReader(in.Body, in.MultipartBoundary) | ||
| contextsPart, err := reader.NextPart() |
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.
This depends on a very specific order of fields in a form, which I don't think we can guarantee. I think it would be better to address the form field via it's name ("tags"). We do something similar when parsing pprof (there's profile and prev_profile fields). I think you can use this method here: https://github.com/pyroscope-io/pyroscope/blob/42ef7de5c83f59d16cd8551552ba4146fa531d38/pkg/parser/parser.go#L206-L216
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.
I rewrote with the same approach as pprof - read whole request into memory up to 32M.
I think I can parse jfr requests without reading whole reqeust in memory
- iterate over multipart fields, if found labels - parse labels, if found jfr - parse jfr
- link labels to jfr & put into storage.
@petethepig let me know if I should do it or it is ok to keep as it is now.
|
Oops, got test timeout on CI. |
79b2120 to
6917434
Compare
Right now tags are sent as protobuf along with JFR as a separate multipart form field.
All events are grouped by contextId to minimize
storage.Putter.PutoperationsOther logic should be the same as previously