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.

Sign up for GitHub

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

Jump to bottom

Adjust tracing configuration to follow the POM model #3570

Merged
merged 4 commits into from Feb 9, 2022
Merged

Adjust tracing configuration to follow the POM model #3570

merged 4 commits into from Feb 9, 2022

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Feb 4, 2022 •
edited

Resolves #3534

@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch from b216ac0 to 1b43f53 Compare February 4, 2022 17:06
@Jimbo4350 Jimbo4350 changed the title Modify tracing defaults to fit the POM model Adjust tracing configuration to fit the POM model Feb 4, 2022
@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch 2 times, most recently from 9fa4b79 to 526f458 Compare February 4, 2022 17:10
@Jimbo4350 Jimbo4350 marked this pull request as ready for review February 4, 2022 17:10
@Jimbo4350 Jimbo4350 changed the title Adjust tracing configuration to fit the POM model Adjust tracing configuration to follow the POM model Feb 4, 2022
@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch from 526f458 to 37b0656 Compare February 4, 2022 18:01
coot
coot requested changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

Very nice, I just would like to request to keep some of the traces on by default (as it was introduced in the commit: 0525536).

cardano-node/src/Cardano/Tracing/Config.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Config.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Config.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Config.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Config.hs Outdated Show resolved Hide resolved
cardano-node/src/Cardano/Tracing/Config.hs Outdated Show resolved Hide resolved
@Jimbo4350
Update proposal comment fix
ec27fc8
@Jimbo4350
Implement PartialTraceOptions and PartialTraceSelection so we can
cbde4a4 
configure the tracers using the POM model
Implement defaultPartialTraceConfiguration
@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch 2 times, most recently from daf816c to e879f82 Compare February 7, 2022 16:05
coot
coot approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM

@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch from e879f82 to 73cb247 Compare February 7, 2022 16:07
@Jimbo4350
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 7, 2022
@iohk-bors @Jimbo4350
Merge #3570
513418d 
3570: Adjust tracing configuration to follow the POM model r=Jimbo4350 a=Jimbo4350

Resolved #3534 

Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
deepfire
deepfire approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@deepfire deepfire left a comment

Choose a reason for hiding this comment

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

The new tracing system is not intended to be toggle-able on a per-tracer basis.

So while it's a cleanup, it's a little misleading -- the config tracer toggles will have no effect, at least for now.

In future, we might still resort to it, but that's not the current intention.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 7, 2022

Build failed:

erikd
erikd approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@erikd erikd left a comment

Choose a reason for hiding this comment

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

Probably want to squash this down.

@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch from 263fabd to 944144a Compare February 8, 2022 20:04
@newhoggy
Copy link
Contributor

newhoggy commented Feb 9, 2022

All the tests appear to fail in the same way:

  ✗ prop_sanityCheck_POM failed at test/Test/Cardano/Node/POM.hs:42:19
    after 1 test.
  
       ┏━━ test/Test/Cardano/Node/POM.hs ━━━
    34 ┃ prop_sanityCheck_POM :: Property
    35 ┃ prop_sanityCheck_POM =
    36 ┃    withTests 1 . Hedgehog.property $ do
    37 ┃     let combinedPartials = defaultPartialNodeConfiguration
    38 ┃                              <> testPartialYamlConfig
    39 ┃                              <> testPartialCliConfig
    40 ┃         nc = makeNodeConfiguration combinedPartials
    41 ┃     case nc of
    42 ┃       Left err -> failWith Nothing $ "Partial Options Monoid sanity check failure: " <> err
       ┃       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       ┃       │ Partial Options Monoid sanity check failure: Default value not specified for TracingVerbosity
    43 ┃       Right config -> config === expectedConfig

@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch 2 times, most recently from 977cb41 to 000a51f Compare February 9, 2022 13:39
@Jimbo4350
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Feb 9, 2022
@iohk-bors @Jimbo4350
Merge #3570
1f5504d 
3570: Adjust tracing configuration to follow the POM model r=Jimbo4350 a=Jimbo4350

Resolves #3534 

Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
@Jimbo4350 Jimbo4350 mentioned this pull request Feb 9, 2022
Closed
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 9, 2022

Build failed:

@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch from 000a51f to 0995966 Compare February 9, 2022 14:50
@Jimbo4350 Jimbo4350 force-pushed the jordan/move-tracing-defaults-to-POM branch from 0995966 to bdfd5ae Compare February 9, 2022 14:53
@Jimbo4350
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 9, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 333c701 into master Feb 9, 2022
@iohk-bors iohk-bors bot deleted the jordan/move-tracing-defaults-to-POM branch February 9, 2022 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikd erikd erikd approved these changes

@deepfire deepfire deepfire approved these changes

@coot coot coot approved these changes

@DavidEichmann DavidEichmann Awaiting requested review from DavidEichmann

@dcoutts dcoutts Awaiting requested review from dcoutts

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@nc6 nc6 Awaiting requested review from nc6

@newhoggy newhoggy Awaiting requested review from newhoggy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] - let one define default on/off values per a tracer in a TraceConfig.
5 participants
@Jimbo4350 @newhoggy @erikd @deepfire @coot