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

fix tracing configuration issues #432

Merged
merged 6 commits into from
Jun 5, 2020
Merged

fix tracing configuration issues #432

merged 6 commits into from
Jun 5, 2020

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented Jun 4, 2020

Prior to this change the configuration options for tracing were controlled exclusively by the tracing_config function in ZebraApp. This resulted in it ignoring all configuration options other than --verbose and more recently ZEBRAD_LOG. Notably, this function is called before the ZebraConfig is ever loaded, so it is impossible to consider the config filter level when initializing the tracing component.

To fix this, we have the new ZebraApp::level fn take the command as input, so it can prioritize the settings correctly. Then we call this function in tracing_config when the Config is None, giving us an initial config, then we call it again in after_config when the Config has been loaded and use that to reload the filter. This change requires a breaking change in the Abscissa API to pass the command along with the config in after_config so they can both be referenced when setting the final level.

fixes #98
fixes #399

@yaahc yaahc requested review from dconnolly and hdevalence June 4, 2020 19:11
@yaahc
Copy link
Contributor Author

yaahc commented Jun 4, 2020

Because this change requires a breaking change to the API of abscissa I'm going to take this opportunity to also fix the Tracing component in abscissa so that it takes a user provided subscriber instead of always setting up its own default subscriber, that way we can setup layers like tracing-error and tracing-flame.

@hdevalence
Copy link
Contributor

@yaahc Just to confirm, the relevant changes to abscissa are here, correct:

iqlusioninc/abscissa@develop...yaahc:develop

@yaahc
Copy link
Contributor Author

yaahc commented Jun 4, 2020

@yaahc Just to confirm, the relevant changes to abscissa are here, correct:

iqlusioninc/abscissa@develop...yaahc:develop

Yup, the last line in that diff is some cruft that snuck in from earlier messing with error handling stuff but the rest of it is all the relevant stuff.

@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #432 into main will decrease coverage by 0.40%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
- Coverage   53.90%   53.49%   -0.41%     
==========================================
  Files          68       68              
  Lines        3278     3303      +25     
==========================================
  Hits         1767     1767              
- Misses       1511     1536      +25     
Impacted Files Coverage Δ
zebrad/src/application.rs 0.00% <0.00%> (ø)
zebrad/src/commands/config.rs 0.00% <0.00%> (ø)
zebrad/src/commands/start.rs 0.00% <0.00%> (ø)
zebrad/src/config.rs 0.00% <0.00%> (ø)
zebra-chain/src/keys/sapling.rs 85.30% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac76d75...de2a14e. Read the comment docs.

zebrad/src/config.rs Outdated Show resolved Hide resolved
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.

🚚


[patch.crates-io]
abscissa_core = { git = "https://github.com/yaahc/abscissa.git", branch = "develop" }
Copy link
Contributor

Choose a reason for hiding this comment

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

If the upsteam is not expected to merge quickly, perhaps make an issue to track 'point to the inqlusion version when available'

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, we'd be happy to merge these changes upstream. Please open a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to but I figured if we're gonna make a breaking change we should be sure that all the breaking changes are done at once, still thinking about changes to error handling.

@yaahc yaahc merged commit 18b4dbc into main Jun 5, 2020
@yaahc yaahc deleted the tracing-config branch June 5, 2020 02:34
teor2345 pushed a commit to teor2345/zebra that referenced this pull request Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants