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

Periodic logging for additional metrics #1094

Merged
merged 5 commits into from
Aug 31, 2022
Merged

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Aug 29, 2022

Should resolve #984

  • chore: enable metrics logging by default
  • feat(logging): add additional logging metrics, change period to 30 seconds

We now log the following fields:

  • Total connections initiated (peer count) since last log
  • Total Messages
  • Total SWAP peers
  • Total FILTER peers
  • Total errors since last log
  • Total subscribed topics

Should the docs mention which fields refer to values over the last reporting period vs since the node setup?

@rymnc rymnc self-assigned this Aug 29, 2022
@rymnc rymnc requested review from jm-clius and LNSD August 29, 2022 06:06
@rymnc
Copy link
Contributor Author

rymnc commented Aug 29, 2022

Additionally, should we log out the metrics using metrics/chronicles_support, as mentioned here, instead of casting them to float64's ourselves?

Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

A little nitpicking.

LGTM ✅

waku/v2/node/wakunode2_setup_metrics.nim Show resolved Hide resolved
waku/v2/node/wakunode2_setup_metrics.nim Outdated Show resolved Hide resolved
@LNSD
Copy link
Contributor

LNSD commented Aug 29, 2022

Tip: Rebase your branch on top of master instead of merging master into your branch to update it with the latest changes. It makes the PRs cleaner.

@rymnc rymnc force-pushed the chore/additional-metrics-logging branch 3 times, most recently from 61d4c91 to 0288fa3 Compare August 30, 2022 04:40
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Good job! Congratulations on your first PR. 🥳 I have added some minor comments on log completion, consistency, etc. If possible, could you add an example log output to this PR to get an idea of how it will look when printed to stdout?

waku/v2/node/wakunode2_setup_metrics.nim Show resolved Hide resolved
waku/v2/node/wakunode2_setup_metrics.nim Outdated Show resolved Hide resolved
waku/v2/node/wakunode2_setup_metrics.nim Outdated Show resolved Hide resolved
waku/v2/node/wakunode2_setup_metrics.nim Outdated Show resolved Hide resolved
@rymnc
Copy link
Contributor Author

rymnc commented Aug 30, 2022

Thanks for your comments, I'll get those changes in tomorrow

@rymnc
Copy link
Contributor Author

rymnc commented Aug 31, 2022

cc: @jm-clius

  • When running wakunode2 with the default config
    image

  • When running wakunode2 with --ports-shift:1 --dns-discovery:true --dns-discovery-url:enrtree://ANTL4SLG2COUILKAPE7EF2BYNL2SHSHVCHLRD5J7ZJLN5R3PRJD2Y@prod.waku.nodes.status.im --discv5-discovery:true
    image

@rymnc rymnc force-pushed the chore/additional-metrics-logging branch from 0288fa3 to 9dd9f2a Compare August 31, 2022 04:02
@rymnc rymnc requested a review from jm-clius August 31, 2022 07:50
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I think this can be merged :)

@rymnc rymnc merged commit ed53bcd into master Aug 31, 2022
@rymnc rymnc deleted the chore/additional-metrics-logging branch August 31, 2022 09:53
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.

Logging: Periodic metric/info dump on running nodes
3 participants