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

Extend gops trace to allow duration parameter #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glibsm
Copy link
Contributor

@glibsm glibsm commented Mar 29, 2022

gops trace :port 10m will now run a trace for 10 minutes,
as opposed to always doing 5 seconds.

For backwards compatibility both server and the client
default to 5s if nothing is specified.

@glibsm
Copy link
Contributor Author

glibsm commented Mar 29, 2022

cc @tklauser

Copy link
Collaborator

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the PR! The code changes LGTM. Could you please update the gops trace usage example in README.md as well?

`gops trace :port 10m` will now run a trace for 10 minutes,
as opposed to always doing 5 seconds.

For backwards compatibility both server and the client
default to 5s if nothing is specified.
@tklauser
Copy link
Collaborator

I've tested this with the following combinations using examples/hello as the agent:

  1. new agent, new gops binary: gops trace <pid> 10s -> works as expected
  2. new agent, old gops binary: gops trace <pid> -> gops hangs indefinitely
  3. old agent, new gops binary
    a. without duration: gops trace <pid> -> gops exits with read tcp 127.0.0.1:53968->127.0.0.1:38565: read: connection reset by peer without displaying a trace
    b. with duration: gops trace <pid> 10s -> same as 3a above.

(where "new" means built from this PR and old means built from current master)

It looks like this change is not backwards compatible. Do you think we can modify it in a way that cases 2 and 3a/b would work as well?

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.

None yet

2 participants