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

node/netmap: Write log messages about NewEpoch events #1763

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

cthulhu-rider
Copy link
Contributor

Within the issue I made a hypothesis that node had missed some ticks of the epoch timer. However it was difficult to explicitly prove or disprove this assumption.

These changes make storage nodes to log new epochs. From now it'll be easy to track the node timeline.

@cthulhu-rider cthulhu-rider added the neofs-storage Storage node application issues label Sep 7, 2022
@cthulhu-rider cthulhu-rider self-assigned this Sep 7, 2022
@cthulhu-rider cthulhu-rider added this to the v0.32.0 milestone Sep 7, 2022
Leonard Lyubich added 2 commits September 7, 2022 15:41
…n event

Sometimes it is useful to track sidechain events processed by the node.
For example, we need some easy way to look up for log messages about new
epoch notifications.

Make `subscriber.Subscriber` to log names of all events received from
the sidechain notification channel.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
… event

There is a need to have the ability to track NeoFS timeline on storage
nodes. Epochs tick on notifications receipt, so the most obvious way to
know about received epochs is logging the events.

Wrap `morphEvent.ParseNewEpoch` event parser into function which writes
log message about new epoch number.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #1763 (31ecd13) into master (fef4f6d) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1763      +/-   ##
==========================================
+ Coverage   33.09%   33.24%   +0.14%     
==========================================
  Files         351      351              
  Lines       23473    23496      +23     
==========================================
+ Hits         7769     7811      +42     
+ Misses      15053    15032      -21     
- Partials      651      653       +2     
Impacted Files Coverage Δ
cmd/neofs-adm/internal/modules/morph/deploy.go 7.08% <0.00%> (-0.06%) ⬇️
pkg/services/tree/service_neofs.pb.go 4.43% <0.00%> (-0.02%) ⬇️
pkg/services/tree/sync.go 0.00% <0.00%> (ø)
pkg/services/tree/cache.go 0.00% <0.00%> (ø)
pkg/services/tree/types.pb.go 20.97% <0.00%> (ø)
pkg/services/tree/replicator.go 0.00% <0.00%> (ø)
pkg/services/tree/service_grpc.pb.go 0.00% <0.00%> (ø)
...neofs-adm/internal/modules/morph/initialize_nns.go 0.00% <0.00%> (ø)
...fs-adm/internal/modules/morph/initialize_deploy.go 0.00% <0.00%> (ø)
pkg/services/tree/service.pb.go 2.93% <0.00%> (+0.66%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cthulhu-rider cthulhu-rider marked this pull request as ready for review September 7, 2022 11:58
@@ -133,6 +133,10 @@ func (s *subscriber) routeNotifications(ctx context.Context) {
continue
}

s.log.Debug("new notification event from sidechain",
Copy link
Member

Choose a reason for hiding this comment

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

why only the regular notification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What else do we need? New blocks are logged. Other things don't touch epoch ticks.

Copy link
Member

Choose a reason for hiding this comment

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

i just was thinking about that switch context

New blocks are logged.

yes, but not here, not in that switch, so that is why i asked what is the difference b/w that cases. IMO, we should either log notifications on the receiver side or log all the notifications in that switch. but not critical

setNetmapNotificationParser(c, newEpochNotification, func(src *state.ContainedNotificationEvent) (event.Event, error) {
res, err := netmapEvent.ParseNewEpoch(src)
if err == nil {
c.log.Info("new epoch event from sidechain",
Copy link
Member

Choose a reason for hiding this comment

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

do we really need that to be "Info"?

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 think yes, epoch ticks is a main heartbeat of the node, and node admin should always have the ability to track the timeline. Don't you think so?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think epoch events are crucial for NeoFS network to function, so it makes sense to log this with Info.

Copy link
Member

Choose a reason for hiding this comment

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

hm, IMO, that is more like debug. in normal situations i do not expect an admin to be informed about epoch via the logs (via the metrics or the monitoring service -- yes), only if he tries to debug smth. but ok

Copy link
Member

Choose a reason for hiding this comment

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

on the other hand, we have the epoch metric and do not have a block metric. that could be assumed as a criterion for being smth "info"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still don't have strict requirement for log levels. Epoch ticks is the main thing that showa node's heartbits in logs.

@cthulhu-rider cthulhu-rider merged commit 699b534 into nspcc-dev:master Sep 12, 2022
cthulhu-rider pushed a commit that referenced this pull request Sep 12, 2022
Sometimes it is useful to track sidechain events processed by the node.
For example, we need some easy way to look up for log messages about new
epoch notifications.

Make `subscriber.Subscriber` to log names of all events received from
the sidechain notification channel.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
@carpawell carpawell mentioned this pull request Sep 14, 2022
@cthulhu-rider cthulhu-rider deleted the log-sidechain-events branch September 30, 2022 13:30
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
…n event

Sometimes it is useful to track sidechain events processed by the node.
For example, we need some easy way to look up for log messages about new
epoch notifications.

Make `subscriber.Subscriber` to log names of all events received from
the sidechain notification channel.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this pull request Oct 19, 2022
… event

There is a need to have the ability to track NeoFS timeline on storage
nodes. Epochs tick on notifications receipt, so the most obvious way to
know about received epochs is logging the events.

Wrap `morphEvent.ParseNewEpoch` event parser into function which writes
log message about new epoch number.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
neofs-storage Storage node application issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants