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

Move tracers into config .yaml file #443

Closed
Jimbo4350 opened this issue Dec 23, 2019 · 6 comments
Closed

Move tracers into config .yaml file #443

Jimbo4350 opened this issue Dec 23, 2019 · 6 comments
Assignees
Labels
priority medium issues/PRs that SHOULD be addressed. This should be done for the release, but acceptable if it doesn

Comments

@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Dec 23, 2019

Tracers should be specified from the config .yaml file as having them on the command line is cumbersome.

@CodiePP
Copy link
Contributor

CodiePP commented Dec 24, 2019

it probably would make sense to type them like: Maybe Severity. This way we can distinguish whether they are on or off, and what the minimum severity is that will pass the filter on this trace.
what do you think?

@Jimbo4350
Copy link
Contributor Author

Jimbo4350 commented Dec 24, 2019

@CodiePP I was thinking of a dictionary structure per tracer in the yaml file like this:

TraceBlockFetchClient:
    On: True
    Severity: Debug
    Verbosity: MaximumVerbosity

Or we can add an additional constructor to Severity (maybe Off for example) and we can simplify the dictionary to:

TraceBlockFetchClient:
    Severity: Off
    Verbosity: MaximumVerbosity

or

TraceBlockFetchClient:
    Severity: Debug
    Verbosity: MaximumVerbosity

Maybe Severity would fit nicely if we had an Off Severity value or something similar. Maybe a name like TracerSwitchedOff to be painfully obvious.

iohk-bors bot added a commit that referenced this issue Dec 27, 2019
444: CAD-379 Update `nix/cardano-node-service.nix` with tracing arguments r=jbgi a=Jimbo4350

Previously not specifying a tracer did not turn it off, it just changed its severity. Currently if you do not specify a tracer, that tracer is turned off. Therefore this is a temporary fix until #443 is implemented.

Co-authored-by: Jordan Millar <[email protected]>
@CodiePP
Copy link
Contributor

CodiePP commented Dec 30, 2019

I was thinking of a dictionary structure per tracer in the yaml file like this:

TraceBlockFetchClient:
    On: True
    Severity: Debug
    Verbosity: MaximumVerbosity

or enabled: True ? can turn on/off the complete trace
currently, verbosity is a global switch. But, this would give more flexibility if it can be defined per trace.
and "Severity" could be renamed to MinSeverity? it is actually the minimum severity level that passes the filter.
you are right, a structure is much better to describe all these options.

@Jimbo4350
Copy link
Contributor Author

TraceBlockFetchClient:
    Enabled: True
    MinSeverity: Debug
    Verbosity: MaximumVerbosity

Made the changes you suggested above 👍

I'd like to make verbosity definable per trace unless there is a reason not to.

@CodiePP
Copy link
Contributor

CodiePP commented Dec 30, 2019

could there also be a global default for MinSeverity? so if it is not indicated for a tracer, then the default will be applied.
same goes for the verbosity, which could even have an implicit default of the global default: NormalVerbosity.

@Jimbo4350
Copy link
Contributor Author

Jimbo4350 commented Dec 30, 2019

Yep we could define that via Maybe Severity/ Maybe Verbosity. If you get Nothing (parsing an empty field) then we can apply a default Severity/Verbosity. Is this what you have in mind?

@vhulchenko-iohk vhulchenko-iohk modified the milestones: S3 2020-01-02, S4 2020-01-16 Jan 6, 2020
@Jimbo4350 Jimbo4350 added the priority medium issues/PRs that SHOULD be addressed. This should be done for the release, but acceptable if it doesn label Jan 14, 2020
iohk-bors bot added a commit that referenced this issue Jan 14, 2020
447: Move tracers to `.yaml` files r=disassembler a=Jimbo4350

Specifying the tracers on the command line is cumbersome and the consensus is to move to them to the configuration `.yaml` files. 

Relevant: #443 

Co-authored-by: Jordan Millar <[email protected]>
iohk-bors bot added a commit that referenced this issue Jan 14, 2020
447: Move tracers to `.yaml` files r=Jimbo4350 a=Jimbo4350

Specifying the tracers on the command line is cumbersome and the consensus is to move to them to the configuration `.yaml` files. 

Relevant: #443 

Co-authored-by: Jordan Millar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority medium issues/PRs that SHOULD be addressed. This should be done for the release, but acceptable if it doesn
Projects
None yet
Development

No branches or pull requests

3 participants