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

Setup tracing-flame for use profiling zebrad #436

Merged
merged 33 commits into from Aug 5, 2020
Merged

Setup tracing-flame for use profiling zebrad #436

merged 33 commits into from Aug 5, 2020

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented Jun 5, 2020

@yaahc yaahc self-assigned this Jun 9, 2020
@yaahc yaahc marked this pull request as ready for review July 31, 2020 23:36
@yaahc yaahc requested a review from a team July 31, 2020 23:36
Cargo.toml Outdated Show resolved Hide resolved
dconnolly
dconnolly previously approved these changes Aug 2, 2020
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

lgtm!

@dconnolly dconnolly added this to In progress in 🦓 via automation Aug 2, 2020
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for getting this working!

Have you tested it?
I'm not sure if it will work on all platforms, or on all future compiler versions.

zebrad/src/commands/start.rs Outdated Show resolved Hide resolved
zebrad/src/commands/start.rs Outdated Show resolved Hide resolved
🦓 automation moved this from In progress to Review in progress Aug 2, 2020
zebrad/src/application.rs Outdated Show resolved Hide resolved
zebrad/src/application.rs Outdated Show resolved Hide resolved
@yaahc
Copy link
Contributor Author

yaahc commented Aug 5, 2020

I have really strong reservations about the platform-specific code that adds special behavior for Windows and Kubernetes. This is the first platform-specific code we have in the project, and from reading the PR I'm not sure exactly what we get from adding it. On the downside, now we have Windows-specific interrupt handling code -- have we tested that it works? Can we test it on an ongoing basis without having to write platform-specific interrupt testing code?

From the code it looks like the purpose is just to allow Ctrl-C to emulate SIGTERM while developing. But it seems like we could also achieve that purpose without adding any code to Zebra using killall -SIGTERM. Is there another purpose that I'm missing?

@hdevalence the initial implementation didn't have any plaform specific code and just relied entirely on tokio::signal::ctrl_c. I figure that API is probably reliable enough where we don't need to do our own windows testing to prove that it works. I added the platform specific handling based on @teor2345's feedback here #436 (comment)

I don't think switching to only handling SIGTERM is a viable solution. There is no sigterm on windows and I think this would be a huge usability regression vs handling ctrl-c, which is how I'd like to be able to terminate the application when working with it day to day. With signals there's a limit to how much we can abstract the behaviour independent of platforms given that windows and linux handle signals and termination of applications completely differently AFAIK. With tokio::signal::ctrl_c that platform specific handling is just in a dependency instead of in our code, and we don't get SIGTERM handling which is likely quite valuable as noted by teor. I think the current implementation is likely reasonably reliable, given that it is vendored from linkerd, and if the ctrl-c handling on windows is broken I don't see that as a huge risk, given that this will just leave us in the same situation we're already in currently where there's no way to gracefully shut down zebrad on windows.

@yaahc yaahc requested a review from teor2345 August 5, 2020 20:50
teor2345
teor2345 previously approved these changes Aug 5, 2020
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great!

I'd like us to document the new exit codes, but otherwise I think we're fine here.

match shutdown {
Shutdown::Graceful => process::exit(0),
Shutdown::Forced => process::exit(1),
Shutdown::Crash => process::exit(2),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not asking for an implementation change here. I don't think having specific return codes for each signal is useful or important for zebrad.

But these new return codes should be documented somewhere in the book.

Let's also document that unhandled signals still use the default return codes.

zebrad/src/application.rs Outdated Show resolved Hide resolved
zebrad/src/config.rs Show resolved Hide resolved
zebrad/src/application.rs Show resolved Hide resolved
🦓 automation moved this from Review in progress to Reviewer approved Aug 5, 2020
🦓 automation moved this from Reviewer approved to Review in progress Aug 5, 2020
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think the default log level has to be "warn", for commands that use standard output.

Otherwise looks good.

@yaahc yaahc requested a review from teor2345 August 5, 2020 22:46
@yaahc yaahc dismissed teor2345’s stale review August 5, 2020 23:07

comment was a non issue

🦓 automation moved this from Review in progress to Reviewer approved Aug 5, 2020
teor2345
teor2345 previously approved these changes Aug 5, 2020
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, just one doc tweak, then I think we're done.

Co-authored-by: teor <teor@riseup.net>
🦓 automation moved this from Reviewer approved to Review in progress Aug 5, 2020
🦓 automation moved this from Review in progress to Reviewer approved Aug 5, 2020
Copy link
Contributor

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

I don't think that the structure of the tracing setup code is very good but as noted in this comment #436 (comment) I think that this awkwardness is forced by our previous decision to upstream our tracing component, which we now have no control over. I would like to try to fix this and "componentize" many of the other parts we currently construct ad-hoc in a later PR.

I still have concerns about the platform-specific behavior but I don't want to block this PR on it.

@yaahc yaahc merged commit 867dd0b into main Aug 5, 2020
🦓 automation moved this from Reviewer approved to Done Aug 5, 2020
@yaahc yaahc deleted the flame branch August 5, 2020 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
🦓
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants