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

Expose node component management #6769

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Nov 27, 2024

I wanted to use the framework for composing a node that we have in the gateway without duplicating a lot of the code onflow/flow-evm-gateway#682

The changes I needed were:

  • FlowNodeImp has *NodeConfig which I did not want to use so I pulled out a sub component NodeImp that does not have that. This forced me to change some log lines. All the info is still there, but it is in the fields instead of the message.
  • moving the ctx, cancel := context.WithCancel(context.Background()) from Run to run is not strictly necessary, but it was a bit confusing so I refactored it (I can potentially put this in a separate PR)
  • Next thing I needed was the handleComponent from FlowNodeBuilder so I changed it into a function that accepts components and a component builder + the input AddWorkersFromComponents. On Flow nodes the input is *NodeConfig, but with making ReadyDoneFactory generic the input can be anything.
  • Context was added to Node.Run for ease of testing. Closing the context will trigger the shutdown of the node, just like a SIGQUIT
  • Last change is strictly not related, and I can put it in a separate PR: I changed the metrics server into a component so that the context can be injected at the start of the metrics server.

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 83.54430% with 13 lines in your changes missing coverage. Please review.

Project coverage is 41.01%. Comparing base (aedb8dc) to head (e6fb7ad).

Files with missing lines Patch % Lines
cmd/scaffold.go 95.34% 2 Missing ⚠️
cmd/access/main.go 0.00% 1 Missing ⚠️
cmd/collection/main.go 0.00% 1 Missing ⚠️
cmd/consensus/main.go 0.00% 1 Missing ⚠️
cmd/execution/main.go 0.00% 1 Missing ⚠️
cmd/ghost/main.go 0.00% 1 Missing ⚠️
cmd/node_builder.go 50.00% 1 Missing ⚠️
cmd/observer/main.go 0.00% 1 Missing ⚠️
cmd/verification/main.go 0.00% 1 Missing ⚠️
insecure/cmd/access/main.go 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6769   +/-   ##
=======================================
  Coverage   41.00%   41.01%           
=======================================
  Files        2103     2103           
  Lines      184990   184995    +5     
=======================================
+ Hits        75862    75873   +11     
+ Misses     102764   102762    -2     
+ Partials     6364     6360    -4     
Flag Coverage Δ
unittests 41.01% <83.54%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

looks good. one small naming suggestion.

Thank you for upgrading the metrics server. I think that should go in a separate PR though

cmd/scaffold.go Outdated
name: name,
func (fnb *FlowNodeBuilder) Component(name string, f ReadyDoneFactory[*NodeConfig]) NodeBuilder {
fnb.components = append(fnb.components, NamedComponentFunc[*NodeConfig]{
FN: f,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about calling this initialize or construct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed. Let me know if it is better.

module/metrics/server.go Outdated Show resolved Hide resolved
Comment on lines +61 to +62
Str("node_role", cfg.BaseConfig.NodeRole).
Hex("spork_id", logging.ID(cfg.SporkID)).
Copy link
Member

Choose a reason for hiding this comment

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

This change would cause every single line of log to include this two fields, is it necessary?
To me, the node role and spork id does not change after startup, so they only need to be logged once, as long as they can be found easily, we don't have to include them in other logs.

I would suggest to move the role and spork id info back to node startup complete log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this logger is used only in this file, which is like 4 log lines. I don't thing the impact is that big.

@janezpodhostnik janezpodhostnik force-pushed the janez/expose-node-component-management branch from bacae24 to 8b0dff9 Compare December 11, 2024 20:47
@janezpodhostnik janezpodhostnik changed the base branch from master to janez/metrics-server-as-component December 11, 2024 20:48
Base automatically changed from janez/metrics-server-as-component to master December 20, 2024 16:38
@janezpodhostnik janezpodhostnik added this pull request to the merge queue Dec 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 20, 2024
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