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

telemetry message "system.network_state" is not being sent in the right format #1901

Closed
kishansagathiya opened this issue Oct 15, 2021 · 5 comments · Fixed by #2078
Closed
Assignees

Comments

@kishansagathiya
Copy link
Contributor

kishansagathiya commented Oct 15, 2021

Describe the bug

To confirm that "system.network_state" is not received in the right format, do the following

  • Setup a local telemetry server using substrate-telemetry.

Use the branch notify-finalized from my fork https://github.com/kishansagathiya/substrate-telemetry (It print the message received, type of message and error. From what I have played around upstream doesn't do that.)

Run telementry backend (telemetry_core and telemetry_shard).

  • Run gossamer by setting telemetry url to the local server.

Run ./bin/gossamer --chain polkadot --name 'Kishans Test Node' --telemetry-url 'ws://localhost:8001/submit 0'

  • Wait for message to get send and received. Follow logs of telemetry_shard command. It will print messages as they are received. Wait for a message with system.network_state in it.
    In the next line you should see the following error
could not parse json into nodeMessage data did not match any variant of untagged enum NodeMessage

Expected Behavior

Current Behavior

Possible Solution

To Reproduce

Steps to reproduce the behaviour:

Log output

Log Output
paste log output...

Specification

  • go version:
  • gossamer version:
  • gossamer commit tag:
  • gossamer commit hash:
  • operating system:
  • additional links:
@kishansagathiya
Copy link
Contributor Author

@edwardmack @timwu20 @noot

I don't see mention of message type in substrate-telemetry. Should i remove this message? Am I missing anything?

@timwu20
Copy link
Contributor

timwu20 commented Nov 2, 2021

Looks like that message isn't encoded correctly? A quick look into the substrate codebase links to an issue and associated PR.

https://github.com/paritytech/substrate/pull/8001/files
paritytech/polkadot-sdk#558

Looks like this message is now renamed to system_unstable_networkState, and our implementation differs from the substrate codebase.

@kishansagathiya
Copy link
Contributor Author

kishansagathiya commented Nov 3, 2021

substrate uses a macro called telemetry to send telemetry messages. I searched for telemetry! and an didn't see any message being sent of type similar to network state.

Here is the list of telemetry messages,
https://github.com/paritytech/substrate-telemetry/blob/6e2faa43be15d34f4e7e1483364c98be4fcef927/backend/telemetry_shard/src/json_message/node_message.rs#L68-L95
There also I don't see anything similar.

@timwu20 It looks like that PR is about renaming rpc method and not telemetry message.

There is a mention of telemetry in that PR in a comment

Nodes send this "network state" to the telemetry, which then shows it in the UI.
The telemetry however doesn't do any JSON-RPC call (unless you're talking about a different telemetry), so it's not affected.

But, by that they mean that node are sending local peed id ( what they called network state earlier) to telemetry. But that happens using a different telemetry message.

@kishansagathiya
Copy link
Contributor Author

kishansagathiya commented Nov 9, 2021

Looks like this message was being sent earlier, but not anymore. They have removed it.
paritytech/substrate#8999. We should probably do the same and network state should instead be sent to Prometheus.

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

🎉 This issue has been resolved in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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 a pull request may close this issue.

3 participants