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

apply additional command overrides based on report format #381

Merged
merged 5 commits into from
May 30, 2018

Conversation

nolanmar511
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented May 24, 2018

Codecov Report

Merging #381 into master will not change coverage.
The diff coverage is 80%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #381   +/-   ##
=======================================
  Coverage   67.08%   67.08%           
=======================================
  Files          36       36           
  Lines        7532     7532           
=======================================
  Hits         5053     5053           
  Misses       2078     2078           
  Partials      401      401
Impacted Files Coverage Δ
internal/driver/driver.go 70.43% <80%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 403a828...210573d. Read the comment docs.

@@ -149,7 +151,7 @@ func generateReport(p *profile.Profile, cmd []string, vars variables, o *plugin.
return out.Close()
}

func applyCommandOverrides(cmd []string, v variables) variables {
func applyCommandOverrides(cmd []string, outputFormat int, v variables) variables {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about changing the first argument to be just "cmd string"? It doesn't seem to be necessary to pass the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -176,6 +178,12 @@ func applyCommandOverrides(cmd []string, v variables) variables {
v.set("nodecount", "80")
}
}

if outputFormat == report.Proto {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you keep case "proto", "raw": in the switch, isn't it redundant now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now redundant. Done.

@@ -176,6 +178,12 @@ func applyCommandOverrides(cmd []string, v variables) variables {
v.set("nodecount", "80")
}
}

if outputFormat == report.Proto {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Side question: why does "peek" case resets the filter but not not the tagfilter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does seem odd. My best guess is that tagfilter was added later, and this is an oversight, but I'll look into the commit history to see if there's evidence for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we should fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tag filtering was added after the peek command, (in this PR), which suggests to me this is an oversight.

I've changed it in this PR. Let me know if you think it ought to be a separate PR.

@@ -176,6 +178,12 @@ func applyCommandOverrides(cmd []string, v variables) variables {
v.set("nodecount", "80")
}
}

if outputFormat == report.Proto {
trim, tagfilter, filter = false, false, false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps for future unless there is an easy way to do it: it would be good to have a way to generate reports in the proto format keeping the filter. E.g. using the internal -flame command with a -tagfocus filter to upload a subset of the profile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming that this could be done just by not setting filter and tagfilter to false. Not sure if we would want to also, for example, add a comment to the profile to indicate filtering was done.

Will experiment, then look into changing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be done later.

@nolanmar511
Copy link
Contributor Author

PTAL

@aalexand
Copy link
Collaborator

@nolanmar511 Appveyor build failed?

@nolanmar511
Copy link
Contributor Author

Appveyor has an 503 error when installing graphviz with chocolatey.

I've retried and am still getting this error. Not sure what's up, though I thinking waiting a bit could help (but that's maybe not the best solution).

@nolanmar511 nolanmar511 merged commit 1ddc9e2 into google:master May 30, 2018
@aalexand
Copy link
Collaborator

@nolanmar511 It would be good to comment on the PR before merging about how the Appveyor failure was fixed - did you do something? did it just go away? something else?

KISSMonX pushed a commit to KISSMonX/pprof that referenced this pull request Jun 14, 2018
* 'master' of github.com:google/pprof: (32 commits)
  record diff base profile label key/value constant (google#384)
  apply additional command overrides based on report format (google#381)
  profile: fix legacy format Go heap profile parsing (google#382)
  add -diff flag for better profile comparision (google#369)
  Use -u flag in pprof installation command so that it updates if needed. (google#376)
  Add GetBase support for ASLR kernel mappings (google#371)
  Add show_from profile filter. (google#372)
  Update README.md due to 8dff45d (google#375)
  Remove stale docs, move useful ones. (google#374)
  internal/driver: skip tests requiring tcp on js (google#373)
  Add "trim path" option which can be used to relocate sources. (google#366)
  Move update_d3flamegraph.sh script from the root directory. (google#370)
  Skip unsymbolizable mapping during symbolz pass. (google#368)
  remove -positive_percentages flag (google#365)
  Render icicle graph in "Flame Graph" view (google#367)
  Add command-line editing support for interactive pprof (google#362)
  Improve profile filter unit tests, fix show filtering of mappings (google#354)
  Fake mappings should be merged into a single one during merging (google#357)
  Hack the code so that both existing Go versions and current Go master format it the same (google#358)
  moved filter tests into their own test file which matches the implementation file, filter.go. (google#356)
  ...
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
* apply additional command overrides based on report format

* address comments

* update filtering
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.

4 participants