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 3 commits into
base: janez/metrics-server-as-component
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 95.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 40.98%. Comparing base (4c8ac17) to head (8b0dff9).

Files with missing lines Patch % Lines
cmd/scaffold.go 95.34% 2 Missing ⚠️
cmd/node_builder.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@                          Coverage Diff                           @@
##           janez/metrics-server-as-component    #6769       +/-   ##
======================================================================
- Coverage                              42.33%   40.98%    -1.36%     
======================================================================
  Files                                    832     2079     +1247     
  Lines                                  68455   183956   +115501     
======================================================================
+ Hits                                   28982    75396    +46414     
- Misses                                 37040   102259    +65219     
- Partials                                2433     6301     +3868     
Flag Coverage Δ
unittests 40.98% <95.71%> (-1.36%) ⬇️

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
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