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

Read Ref IDs as uint64 #43

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

sivachandran
Copy link
Contributor

Fixes overflow error in parsing Ref IDs

Fixes overflow error in parsing Ref IDs
@korniltsev korniltsev merged commit a8d22a1 into grafana:main Oct 16, 2024
2 checks passed
@sivachandran sivachandran deleted the read-ref-id-as-uint64 branch October 16, 2024 06:17
@sivachandran
Copy link
Contributor Author

Can you tag a release? I need the fix in my usage.

@korniltsev
Copy link
Collaborator

May I ask why can't you go get jfr-parser@a8d22a1 ?

@korniltsev
Copy link
Collaborator

or it doe snot work?

@sivachandran
Copy link
Contributor Author

it works only if i do

go get github.com/grafana/jfr-parser@a8d22a1
go get github.com/grafana/jfr-parser/pprof@a8d22a1

in this case there is no change under pprof. otherwise without the second go get i won't get changes under pprof.

is there a reason to have nested package?

@korniltsev
Copy link
Collaborator

is there a reason to have nested package?

The first one is for parsing, and the other is for pprof producing. They have slightly different dependencies. Furthermore the latter one has a little bit of pyroscope specifics.

@sivachandran
Copy link
Contributor Author

The first one is for parsing, and the other is for pprof producing. They have slightly different dependencies. Furthermore the latter one has a little bit of pyroscope specifics.
I understand.

But considering the fact pprof depends on jfr-parser/parser and can't be used independently why don't we keep everything under single package. It would make the dependency management and releasing simpler.

@korniltsev
Copy link
Collaborator

Cause there may be users who dont need pprof

@sivachandran
Copy link
Contributor Author

Hmm... make sense. Thanks for explaining!

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