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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Beacon cleanups #5148

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - Beacon cleanups #5148

wants to merge 8 commits into from

Conversation

poszu
Copy link
Contributor

@poszu poszu commented Oct 12, 2023

Some clean-ups in beacon code:

  • removed redundant:
    • nodeID (can be derived from edSigner)
    • and metricsRegistry (unused
  • use codec.MustEncode where an error lead to shutdown anyway
  • avoid costly eager marshaling for log purposes

馃挕 It's recommended to review commit-by-commit as changes are separated.

beacon/beacon.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #5148 (076d1da) into develop (c699075) will increase coverage by 0.0%.
Report is 1 commits behind head on develop.
The diff coverage is 65.5%.

@@           Coverage Diff           @@
##           develop   #5148   +/-   ##
=======================================
  Coverage     77.5%   77.5%           
=======================================
  Files          258     258           
  Lines        30391   30342   -49     
=======================================
- Hits         23566   23533   -33     
+ Misses        5320    5313    -7     
+ Partials      1505    1496    -9     
Files Coverage 螖
beacon/handlers.go 87.1% <100.0%> (+0.7%) 猬嗭笍
beacon/state.go 100.0% <100.0%> (酶)
beacon/weakcoin/weak_coin.go 80.3% <100.0%> (+2.0%) 猬嗭笍
node/node.go 63.7% <100.0%> (酶)
beacon/beacon.go 80.8% <87.5%> (+1.5%) 猬嗭笍
beacon/message.go 72.7% <0.0%> (-27.3%) 猬囷笍
beacon/proposal_list.go 75.0% <28.5%> (-10.8%) 猬囷笍
beacon/proposal_set.go 28.5% <28.5%> (-71.5%) 猬囷笍
beacon/votes_calc.go 88.6% <44.4%> (-11.4%) 猬囷笍

... and 1 file with indirect coverage changes

@@ -237,7 +225,7 @@ func (pd *ProtocolDriver) Start(ctx context.Context) {
pd.logger.Info("beacon protocol disabled")
return
}
pd.logger.With().Info("starting beacon protocol", log.String("config", fmt.Sprintf("%+v", pd.config)))
pd.logger.With().Info("starting beacon protocol", log.Any("config", pd.config))
Copy link
Member

@fasmat fasmat Oct 12, 2023

Choose a reason for hiding this comment

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

I think using log.Any is expensive, because zap needs to go over the object with reflection to generate a string from it. I'd prefer if pd.config would implement zapcore.ObjectMarshaler by adding a MarshalLogObject method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally yes, but this is a Start() method that is executed once per application life, so IMHO it doesn't make sense to bloat code with special implementation just for this use-case.

beacon/beacon.go Outdated Show resolved Hide resolved
beacon/proposal_set.go Outdated Show resolved Hide resolved
@poszu
Copy link
Contributor Author

poszu commented Oct 12, 2023

Bors merge

bors bot pushed a commit that referenced this pull request Oct 12, 2023
Some clean-ups in beacon code:
- removed redundant:
  -  nodeID (can be derived from edSigner)
  -  and metricsRegistry (unused
- use `codec.MustEncode` where an error lead to shutdown anyway
- avoid costly eager marshaling for log purposes

:bulb: It's recommended to review commit-by-commit as changes are separated.
@bors
Copy link

bors bot commented Oct 12, 2023

Pull request successfully merged into develop.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Beacon cleanups [Merged by Bors] - Beacon cleanups Oct 12, 2023
@bors bors bot closed this Oct 12, 2023
@bors bors bot deleted the beacon-cleanups branch October 12, 2023 16:16
dshulyak pushed a commit to dshulyak/go-spacemesh that referenced this pull request Oct 13, 2023
Some clean-ups in beacon code:
- removed redundant:
  -  nodeID (can be derived from edSigner)
  -  and metricsRegistry (unused
- use `codec.MustEncode` where an error lead to shutdown anyway
- avoid costly eager marshaling for log purposes

:bulb: It's recommended to review commit-by-commit as changes are separated.
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.

None yet

2 participants