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

CAD-379 Update nix/cardano-node-service.nix with tracing arguments #444

Merged
merged 1 commit into from
Dec 27, 2019

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Dec 23, 2019

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.

@Jimbo4350 Jimbo4350 force-pushed the jordan/tracing-fix-cardano-node-service branch from 60b7e23 to 5058945 Compare December 23, 2019 19:04
@Jimbo4350 Jimbo4350 marked this pull request as ready for review December 23, 2019 19:05
Copy link
Contributor

@CodiePP CodiePP left a comment

Choose a reason for hiding this comment

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

how can this be tested?

scripts/shelley-testnet2.sh Show resolved Hide resolved
@Jimbo4350 Jimbo4350 force-pushed the jordan/tracing-fix-cardano-node-service branch from 5058945 to 91780d0 Compare December 23, 2019 19:20
@Jimbo4350
Copy link
Contributor Author

Jimbo4350 commented Dec 23, 2019

how can this be tested?

I'm not sure, running the output of nix-build -A scripts.staging.node on master gives the following error:

cardano-node: Error in $: expected [a], encountered Object
CallStack (from HasCallStack):
  error, called at src/Cardano/Node/Run.hs:155:14 in cardano-node-1.2.0-LOpCdC5zsAm4JmQZiPafiR:Cardano.Node.Run%                                              

The output script on my branch does look correct however.

The scripts successfully passing in hydra should confirm that I didn't break anything but I'll need somebody from devops to confirm the logging output is what they want.

Copy link
Contributor

@disassembler disassembler left a comment

Choose a reason for hiding this comment

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

These shouldn't be here. They should be argument in cmd of mkScript instead.

https://github.com/input-output-hk/cardano-node/blob/jordan/tracing-fix-cardano-node-service/nix/nixos/cardano-node-service.nix#L27

You can test with curl localhost:12798/metrics|grep cardano. If you see the metrics, it works.

@Jimbo4350 Jimbo4350 force-pushed the jordan/tracing-fix-cardano-node-service branch from 91780d0 to cb00aa9 Compare December 23, 2019 20:58
@Jimbo4350 Jimbo4350 changed the title Update nix/cardano-node-service.nix with tracing arguments CAD-370 Update nix/cardano-node-service.nix with tracing arguments Dec 27, 2019
@Jimbo4350 Jimbo4350 changed the title CAD-370 Update nix/cardano-node-service.nix with tracing arguments CAD-379 Update nix/cardano-node-service.nix with tracing arguments Dec 27, 2019
@Jimbo4350 Jimbo4350 force-pushed the jordan/tracing-fix-cardano-node-service branch from cb00aa9 to 95d9fb2 Compare December 27, 2019 15:21
@jbgi jbgi self-requested a review December 27, 2019 15:55
@jbgi
Copy link
Contributor

jbgi commented Dec 27, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request 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]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 27, 2019

@iohk-bors iohk-bors bot merged commit 95d9fb2 into master Dec 27, 2019
@iohk-bors iohk-bors bot deleted the jordan/tracing-fix-cardano-node-service branch December 27, 2019 16:42
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.

4 participants