Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Doc fixes for sc-telemetry & API struct rename #7934

Merged
11 commits merged into from
Jan 29, 2021
Merged

Doc fixes for sc-telemetry & API struct rename #7934

11 commits merged into from
Jan 29, 2021

Conversation

cecton
Copy link
Contributor

@cecton cecton commented Jan 20, 2021 •
edited
Loading

Related to #7463

polkadot companion: paritytech/polkadot#2348

@cecton cecton requested a review from dvdplm January 20, 2021 12:34
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 20, 2021
@cecton
Copy link
Contributor Author

cecton commented Jan 20, 2021

@dvdplm Thanks a lot for the review! You can continue 🙏

@cecton
Copy link
Contributor Author

cecton commented Jan 20, 2021

…but I think the booleans should be flipped so we'd have logger.with_log_reloading(self.log_reloading).

The origin of these funny booleans isn't this PR, but maybe we can avoid extending the awkwardness and have to do one bool flip here, and then another one in GlobalLoggerBuilder::with_log_reloading(). I'd rename GlobalLoggerBuilder.disable_log_reloading to just log_reloading: bool and work backwards from there.

It all comes down to the CLI. In the cli we have a parameter --disable-log-reloading. It's good as it is because the normal behavior is to have the log reloading and then the user have to insert --disable-log-reloading if they want to deactivate it.

On the builder though it makes more sense to have positive function. That is why I used with_log_reloading even though the default is to have it enabled.

I'm not sure why I kept disable_log_reloading internally 🤔 this could be changed to just log_reloading I guess.

@cecton
Copy link
Contributor Author

cecton commented Jan 20, 2021

@dvdplm I just realized now that the macro that was supposed to disable the log reloading was actually enabling the log reloading 😆

So I fixed that at the same time.

cecton
cecton commented Jan 22, 2021
View reviewed changes
client/service/src/config.rs
_ => None,
}
// Don't initialise telemetry if `telemetry_endpoints` == Some([])
self.telemetry_endpoints.as_ref().filter(|x| !x.is_empty())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh there it is @dvdplm . See! I knew I did it. It was just in another PR

@cecton cecton marked this pull request as ready for review January 22, 2021 20:09
@cecton cecton requested a review from bkchr January 22, 2021 20:09
@cecton
Copy link
Contributor Author

cecton commented Jan 22, 2021

This is also ready for review

@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 22, 2021
dvdplm
dvdplm approved these changes Jan 25, 2021
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lovely, ty.

bkchr
bkchr approved these changes Jan 25, 2021
View reviewed changes
cecton and others added 2 commits January 27, 2021 12:33
@cecton @bkchr
Update client/tracing/src/logging/directives.rs
daa6021 
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@cecton
Merge commit 0b0d124 (no conflict)
7c5913c
@cecton cecton changed the title Doc fixes for sc-telemetry Doc fixes for sc-telemetry & API function rename Jan 29, 2021
@cecton cecton changed the title Doc fixes for sc-telemetry & API function rename Doc fixes for sc-telemetry & API struct rename Jan 29, 2021
@cecton cecton mentioned this pull request Jan 29, 2021
Merged
@cecton cecton added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 29, 2021
@cecton
Copy link
Contributor Author

cecton commented Jan 29, 2021

bot merge

@ghost
Copy link

ghost commented Jan 29, 2021

Trying merge.

@ghost ghost merged commit a51a988 into master Jan 29, 2021
@ghost ghost deleted the cecton-telemetry-doc-fixes branch January 29, 2021 10:57
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cecton @dvdplm @bkchr