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

Refactoring of CLI options #277

Merged
merged 4 commits into from
Jun 28, 2023
Merged

Refactoring of CLI options #277

merged 4 commits into from
Jun 28, 2023

Conversation

r1viollet
Copy link
Collaborator

@r1viollet r1viollet commented Jun 19, 2023

What does this PR do?

  • Restructure the documentation
  • Add a CLI library to parse arguments
    This makes it safer and ensures a proper error message is surfaced to users.
  • Remove C types in favor of C++ types

⚠️ breaking change for users

Some of the options are now considered as flags (no longer require a boolean value to be given):
Example:

ddprof --global yes
# Should be replaced by
ddprof --global

Motivation

  • ddprof's help was too complicated.
  • Prior to changing some of the watcher configuration, I wanted to review our current documentation.
  • This issue Configurable sampling frequency #248 was requesting control over the frequency setting
    This was not documented. I'm adding a basic documentation of events.

Additional Notes

This will require users to adjust some of their command lines

How to test the change?

I re-adjusted all the unit tests to the new code paths.
Downstream test PR
https://github.com/DataDog/ddprof-build/pull/100

@r1viollet r1viollet force-pushed the r1viollet/ddprof_cli branch 5 times, most recently from 1e69537 to 199cc90 Compare June 20, 2023 13:08
src/ddprof_worker.cc Fixed Show fixed Hide fixed
src/ddprof_worker.cc Fixed Show resolved Hide resolved
@r1viollet r1viollet force-pushed the r1viollet/ddprof_cli branch from 2aa1b2a to 298c39e Compare June 20, 2023 15:50
test/ddprof_input-ut.cc Outdated Show resolved Hide resolved
@r1viollet r1viollet force-pushed the r1viollet/ddprof_cli branch from 1993195 to 4b7e337 Compare June 21, 2023 10:58
@r1viollet r1viollet changed the title [ongoing] Refactoring of CLI options Refactoring of CLI options Jun 21, 2023
src/ddprof_context_lib.cc Outdated Show resolved Hide resolved
@r1viollet r1viollet marked this pull request as ready for review June 21, 2023 11:03
@r1viollet r1viollet force-pushed the r1viollet/ddprof_cli branch 2 times, most recently from c6e0a1f to b6eb5ff Compare June 22, 2023 09:43
@r1viollet r1viollet force-pushed the r1viollet/ddprof_cli branch 4 times, most recently from 1a5d395 to 5ec7cb7 Compare June 26, 2023 16:34
src/ddprof.cc Outdated Show resolved Hide resolved
src/ddprof_cli.cc Outdated Show resolved Hide resolved
src/ddprof_cli.cc Outdated Show resolved Hide resolved
src/ddprof_cli.cc Outdated Show resolved Hide resolved
src/exe/main.cc Show resolved Hide resolved
src/presets.cc Outdated Show resolved Hide resolved
src/statsd.cc Outdated Show resolved Hide resolved
src/statsd.cc Outdated Show resolved Hide resolved
src/tags.cc Outdated Show resolved Hide resolved
src/version.cc Outdated Show resolved Hide resolved
- Restructure the documentation
- Add CLI11 as a library to parse arguments
This makes it safer and ensures a proper error message is surfaced to users.
- Remove C types in favor of C++ types
- Introduce configuration files
@r1viollet r1viollet force-pushed the r1viollet/ddprof_cli branch from 8c7b75e to de72cee Compare June 28, 2023 07:39
- Add bound checks to string manipulations
- Add tag APIs to handle string views
test/simple_malloc-ut.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@nsavoire nsavoire left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for taking the time to improve input options !!

@r1viollet r1viollet merged commit 38b8d30 into main Jun 28, 2023
@r1viollet r1viollet deleted the r1viollet/ddprof_cli branch June 28, 2023 12:36
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.

3 participants