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

Add operator registration metric at node #502

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

jianoaix
Copy link
Contributor

@jianoaix jianoaix commented Apr 19, 2024

Why are these changes needed?

Provide metrics to help address major operator pain points

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

node/metrics.go Outdated Show resolved Hide resolved
},
[]string{"quorum", "type"},
Copy link
Contributor

Choose a reason for hiding this comment

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

wait I am very confused with this metrics.
This says Registered which seems, operator is registered or not. So binary 0 and 1

but we are setting this metrics to stake_share and rank. I think it will create confusion and probably not a right use of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

instead I think the better way can be - have 3 diff metrics

  1. stake_share and for that label would be quorum
  2. rank and for that label would be quorum
  3. registered for that label would be quorum

so basically you just slice by quorum for all 3 metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means the stake registered for each quorum: one for the precentage info, the other for the rank by the stake
I'm not sure why it's confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

coz the metrics name is registered so this means- we are emitting the value of registered. but here the value is actually of a label and that's confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

like for example

node_eigenda_blobs_total{quorum="0",type="number"} 97605
node_eigenda_blobs_total{quorum="0",type="size"} 1.16567530656e+11

the metrics value is of node_eigenda_blobs_total - and then you can slice by quorum and type to identify more params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You just have 2 time series per quorum: the stake share and rank. The same for node_eigenda_blobs_total.
There is no confusion point, no inefficiency as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

image
from https://prometheus.io/docs/practices/naming/

Madhur makes good points and I feel it would be clearer if we move these labels into their own metrics time series.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting, this may affect a lot of metrics we built

Copy link
Contributor

Choose a reason for hiding this comment

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

yea I know. but I think we should at least start making sure new metrics are correct and eventually migrate other faulty ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you feel strongly about it, feel free to open a PR for it, I'll approve it

@jianoaix jianoaix merged commit 898eb10 into Layr-Labs:master Apr 19, 2024
5 checks passed
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.

3 participants