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

metrics: expose seastar runtime metric #5845

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

VladLazar
Copy link
Contributor

Cover letter

Expose the "scheduler_runtime_ms" metric on the "public_metrics" endpoint.

Backport Required

  • not a bug fix
  • papercut/not impactful enough to backport
  • v22.2.x
  • v22.1.x
  • v21.11.x

UX changes

  • none

Release notes

  • none

Expose the "scheduler_runtime_ms" metric on the "public_metrics"
endpoint.
@VladLazar VladLazar requested a review from BenPope August 5, 2022 10:17
@VladLazar VladLazar marked this pull request as ready for review August 5, 2022 10:17
Copy link
Member

@mmaslankaprv mmaslankaprv left a comment

Choose a reason for hiding this comment

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

lgtm, one question do we have uptime metric in the new endpoint ?

@VladLazar
Copy link
Contributor Author

lgtm, one question do we have uptime metric in the new endpoint?

Good point. We don't, but we probably should. I'll create a PR.

Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

I was hoping to have the new metrics follow best practice of using base SI units, reporting in ms is disappointing.

@VladLazar
Copy link
Contributor Author

I was hoping to have the new metrics follow best practice of using base SI units, reporting in ms is disappointing.

I agree it's not ideal. Seastar doesn't currently expose that through the public interface. We could patch it though.

@BenPope
Copy link
Member

BenPope commented Aug 5, 2022

I was hoping to have the new metrics follow best practice of using base SI units, reporting in ms is disappointing.

I agree it's not ideal. Seastar doesn't currently expose that through the public interface. We could patch it though.

I'd rather add code to be able to map the value and push this feature to the next minor release than publish it with ms. We should do the right thing.

@VladLazar
Copy link
Contributor Author

I was hoping to have the new metrics follow best practice of using base SI units, reporting in ms is disappointing.

I agree it's not ideal. Seastar doesn't currently expose that through the public interface. We could patch it though.

I'd rather add code to be able to map the value and push this feature to the next minor release than publish it with ms. We should do the right thing.

I hacked something together today. I'll create pull requests on Monday.

@dotnwat dotnwat merged commit e530d26 into redpanda-data:dev Aug 5, 2022
@VladLazar
Copy link
Contributor Author

I created a couple PRs for reporting the same metric, but in seconds. It requires exposing the data from seastar first. If we find this approach preferable, we should revert this merge.

@BenPope have a look and let me know what you think. I've maked them as drafts for now to prevent them from being merged:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants