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

export the driver/report/measurement subpackage #496

Closed
wants to merge 1 commit into from
Closed

export the driver/report/measurement subpackage #496

wants to merge 1 commit into from

Conversation

lonng
Copy link

@lonng lonng commented Oct 30, 2019

Signed-off-by: Lonng heng@lonng.org

  1. This PR export some internal subpackage to allow the external library to reuse this library easier.
    e.g: perfschema: support query cpu/memory/mutex/block/allocs/goroutines flamegraph by SQL (#12986) pingcap/tidb#13009
  2. Add go module support.

Signed-off-by: Lonng <heng@lonng.org>
@@ -0,0 +1,11 @@
module github.com/google/pprof
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to include these files? This doesn't seem to be related to this PR.

Copy link
Author

Choose a reason for hiding this comment

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

It's better to add go module support. But I can delete this file if there is not go module support plan.

Copy link
Author

Choose a reason for hiding this comment

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

@aalexand How do you think?

@nolanmar511
Copy link
Contributor

I'm really hesitant about making anything that's internal no longer internal for two reasons. First, it will have an impact on how easy/hard it is to make changes to the pprof CLI. I've definitely made a range of changes to pprof where I broke the APIs of methods that are in "internal", but would not be in "internal" following this change. I'm particularly concerned about making files in "report" no longer internal -- the Options structure is tied to all of pprof's options -- if we add or modify any pprof options, it would likely be a breaking change. Next, any sub-package that we move out of internal becomes a sub-package which we must support -- and these sub-packages were not really written with the intention of being used outside of pprof; they have certain characteristics which seem pprof-specific.

It may be a good thing to do; but it is a support burden that I'm not sure our will team have capacity for now and going forward.

@nolanmar511
Copy link
Contributor

If we do decide to make internal packages public, it's something that likely needs to be done with a great degree of care. For example, "github.com/google/pprof/internal/plugin" would continue to be an internal package following this change, but plugin.ObjTool is passed to func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool) (within report.go), and I'd want to look more at details like that.

Given the degree of care that I think a change like this deserves, I'd be for splitting this change in to smaller changes -- perhaps a PR per sub-package being made public; or maybe even splitting sub-packages into private and public sub-packages to minimize the number of functions exposed by this change.

If we do go forward with these changes, I would be in favor go module support, since that will allow us to make breaking changes more easily when necessary (and using go modules might be a good thing to do generally). It would probably be nice to consider that within a separate PR.

@aalexand
Copy link
Collaborator

@lonng Looking closer I think I agree with @nolanmar511. It seems the only function used in pingcap/tidb#13009 is measurement.Percentage and the implementation of it is pretty simple. So, quoting Go proverbs, "A little copying is better than a little dependency", especially given that making this package public would make us carry the responsibility of any backwardly incompatible changes (we don't have to claim we'd maintain backward compatibility, but the responsibility for making decisions potentially affecting others would still be there).

So, I think I am in favor of dropping this PR and have a little of copying into pingcap repo.

@lonng
Copy link
Author

lonng commented Oct 31, 2019

@aalexand @nolanmar511 Thank you for your reply, I already understand your concerns and give you thumbs up for your rigor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants