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

7296 subnet subscriptions metrics #7411

Merged

Conversation

mehdi-aouadi
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi commented Aug 3, 2023

PR Description

Add subnet subscription metrics:

  • peristent_subnet_x
  • aggregation_subnet_x

Fixed Issue(s)

fixes #7341

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@mehdi-aouadi mehdi-aouadi mentioned this pull request Aug 3, 2023
2 tasks
@mehdi-aouadi
Copy link
Contributor Author

These metrics allow to draw the following graphs:

Screenshot 2023-08-04 at 16 54 43

Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

Correct me if I'm wrong in assumptions.
Say, we have non-persistent subnet subscription at id 9 with unsubscription slot 100. So we have metric non-persistent (9, 1). Next, we get persistent subscription on 90 slot to slot 800, ubsubscription slot will be updated and persistent id will be added to persistent set, but not the metrics. It means next 710 slots it will be displayed as non-persistent. At the end on unsubscription we will find id in persistentId, mark persistent gauge as 0 not touching non-persistent, so metric will remains non-persistent (9, 1) even after unsubscription. So we need something like this:

  1. check if update is > than current unsubscription, turn off old one and turn on new. It will be not 100% correct but way better
  2. on persistent unsubscription put 0 in both labels
    Also we could add a test for it.

@mehdi-aouadi mehdi-aouadi self-assigned this Aug 14, 2023
@mehdi-aouadi
Copy link
Contributor Author

mehdi-aouadi commented Aug 14, 2023

Correct me if I'm wrong in assumptions. Say, we have non-persistent subnet subscription at id 9 with unsubscription slot 100. So we have metric non-persistent (9, 1). Next, we get persistent subscription on 90 slot to slot 800, ubsubscription slot will be updated and persistent id will be added to persistent set, but not the metrics. It means next 710 slots it will be displayed as non-persistent. At the end on unsubscription we will find id in persistentId, mark persistent gauge as 0 not touching non-persistent, so metric will remains non-persistent (9, 1) even after unsubscription. So we need something like this:

  1. check if update is > than current unsubscription, turn off old one and turn on new. It will be not 100% correct but way better
  2. on persistent unsubscription put 0 in both labels
    Also we could add a test for it.

I added an aggregation (non-persistent) to persistent subnet metrics swap when needed.
I implemented the following logic:

  • When not subscribed at all to the subnet and we receive a subscription request:
    • If persistent subnet -> publish persistent subnet metric with value 1
    • If aggregation subnet - > publish aggregation subnet metric with value 1
  • When already subscribed as a persistent subnet and we receive a subscription request with the same subnet id:
    • If the new subscription is for persistent subnet: update the un-subscription slot if > existing one, do nothing for the metrics
    • If the new subscription request is for an aggregation subnet and the un-subscription slot > existing persistent un-subscription slot: Replace the persistent subscription by an aggregation subnet subscription -> Set the persistent subnet metric to 0, set a new aggregation metric with value 1, remove the persistent subscription from the known persistent subscriptions
    • If the new subscription request is for an aggregation subnet and the un-subscription slot <= existing persistent un-subscription slot: Do nothing (we're already subscribed to that subnet as a persistent subnet and the existing un-subscription slot > un-subscription request slot, which means there is no risk to miss the assigned aggregation duty)
  • When we reach an un-subscription slot:
    • If the subnet is persistent: set the persistent subnet metric to 0
    • If the subnet is not persistent: set the aggregation subnet metric to 0

The only drawback of this logic is that the aggregation subnet subscription prevail (and that's ok, we should not miss aggregation duties in any case) which could lead to some situations where we only see aggregation subnet subscription for few slots (when the aggregation subnet un-subscription slot > existing persistent subnet un-subscription slot, in that case we replace the persistent subscription by the aggregation ones).

@mehdi-aouadi mehdi-aouadi force-pushed the 7296-subnet-subscriptions-metrics branch from 7fbd078 to 7799954 Compare August 14, 2023 16:30
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

LGTM

@mehdi-aouadi mehdi-aouadi enabled auto-merge (squash) August 18, 2023 09:16
@mehdi-aouadi mehdi-aouadi enabled auto-merge (squash) August 18, 2023 09:20
@mehdi-aouadi mehdi-aouadi merged commit 7e5e6bc into Consensys:master Aug 18, 2023
12 of 14 checks passed
@mehdi-aouadi mehdi-aouadi deleted the 7296-subnet-subscriptions-metrics branch August 18, 2023 11:47
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.

None yet

2 participants