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.

Sign up for GitHub

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

Jump to bottom

[native] Prestissimo worker metrics documentation #23107

Merged
merged 1 commit into from
Aug 21, 2024
Merged

[native] Prestissimo worker metrics documentation #23107

merged 1 commit into from
Aug 21, 2024

Conversation

karteekmurthys
Copy link
Contributor

Description

Added Prestissimo worker metrics related config documentation.

Motivation and Context

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

@karteekmurthys karteekmurthys requested review from steveburnett and a team as code owners July 1, 2024 17:32
@github-actions github-actions bot added the docs label Jul 1, 2024
@majetideepak
Copy link
Collaborator

Let's add documentation here as well presto-docs/src/main/sphinx/presto_cpp/features.rst

@majetideepak majetideepak changed the title [Native] Prestissimo worker metrics documentation [native] Prestissimo worker metrics documentation Jul 1, 2024
steveburnett
steveburnett requested changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the doc, looks good! Some formatting nits and a couple of text suggestions.

presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/properties.rst Outdated Show resolved Hide resolved
steveburnett
steveburnett requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Minor formatting suggestion for consistency with the first set of HTTP endpoints in lines 16-21.

presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
steveburnett
steveburnett requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

A nit I missed on the previous review, apologies.

presto-docs/src/main/sphinx/presto_cpp/features.rst Outdated Show resolved Hide resolved
steveburnett
steveburnett previously approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)
Pull updated branch, new local doc build, looks good. Thanks!

@aditi-pandit
Copy link
Contributor

@karteekmurthys : Might be good to also give more information about the actual metrics as well. features.rst could be a good place for it. Velox documents https://facebookincubator.github.io/velox/monitoring/metrics.html. We should give more information on how to get these from the metrics collection (show some snapshots from our tooling).

Would be great to document some important new Prestissimo level metrics as well.

@steveburnett
Copy link
Contributor

Suggest updating the release note entry to

== NO RELEASE NOTE ==

yingsu00
yingsu00 reviewed Jul 12, 2024
View reviewed changes
presto-docs/src/main/sphinx/presto_cpp/features.rst
@@ -22,8 +22,8 @@ HTTP endpoints related to tasks are registered to Proxygen in

Other HTTP endpoints include:

* POST: v1/memory
* Reports memory, but no assignments are adjusted unlike in Java workers.
* POST: v1/memory: Reports memory, but no assignments are adjusted unlike in Java workers
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not clear enough. Will you be able to add more explanations here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@karteekmurthys, could you respond to @yingsu00's comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added more details now.

@github-actions github-actions bot removed the docs label Aug 20, 2024
@yingsu00 yingsu00 merged commit dec6c14 into prestodb:master Aug 21, 2024
60 checks passed
aditi-pandit
aditi-pandit reviewed Aug 21, 2024
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Lazy review +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveburnett steveburnett steveburnett left review comments

@aditi-pandit aditi-pandit aditi-pandit left review comments

@yingsu00 yingsu00 yingsu00 approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@majetideepak majetideepak Awaiting requested review from majetideepak

@tdcmeehan tdcmeehan Awaiting requested review from tdcmeehan

@elharo elharo Awaiting requested review from elharo elharo is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Presto Documentation
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@karteekmurthys @majetideepak @aditi-pandit @steveburnett @yingsu00