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

Further decouple SystemInterval values. #289

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

romanb
Copy link
Contributor

@romanb romanb commented Sep 14, 2020

In principle, most values in a SystemInterval can already be reported independently to the backend, which is a good thing. Unfortunately there are still two constraints:

  1. txcount and peers must be sent together.
  2. block information must always be sent with any other information, i.e. it is not possible to send any update that does not include block information.

This unnecessarily restricts the granularity at which values can be sent to the telemetry backend. For example, the txcount and peers actually come from quite different sources in substrate, where either one may be unavailable independently of the other. This either prevents reporting one of these values when the other is not available or requires the code that sends these values to remember the last values of any such fields that are coupled together in this manner, just so an update can be sent.

I think the frontend, i.e. the Telemetry, is the best place to handle this by giving maximum flexibility to the incoming data, meaning as much as possible it does not prescribe which values must be sent together and just updates its local state whenever it gets a new value. Therefore this PR decouples txcount and peers in the SystemInterval message and makes the block field optional.

Context: https://github.com/paritytech/substrate/pull/6986/files#r487127101.

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

Small tidbit, looks fine to me otherwise.

backend/src/node.rs Outdated Show resolved Hide resolved
Co-authored-by: Maciej Hirsz <1096222+maciejhirsz@users.noreply.github.com>
backend/src/node.rs Outdated Show resolved Hide resolved
@romanb romanb merged commit b98c816 into master Sep 14, 2020
@romanb romanb deleted the system-interval-decouple-values branch September 14, 2020 15:43
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.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants