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

Fix typo in metrics document #10374

Closed
wants to merge 1 commit into from
Closed

Fix typo in metrics document #10374

wants to merge 1 commit into from

Conversation

lingbin
Copy link
Contributor

@lingbin lingbin commented Jul 2, 2024 •
edited
Loading

DEFINE_HISTOGRAM_STAT --> DEFINE_HISTOGRAM_METRIC

The name of the macro used to register a histogram-type metric is
DEFINE_HISTOGRAM_METRIC, not DEFINE_HISTOGRAM_STAT, this PR
fixes it.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 2, 2024
@netlify Netlify
Copy link

netlify bot commented Jul 2, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 72b6c38
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/669131d8a81145000841dce8

lingbin
lingbin commented Jul 12, 2024
View reviewed changes
velox/docs/monitoring/metrics.rst
@@ -43,7 +42,7 @@ number of shuffle requests per second.
**Histogram**: tracks the distribution of event data point values, such as query
execution time distribution. The histogram metric divides the entire data range
into a series of adjacent equal-sized intervals or buckets, and then count how
many data values fall into each bucket. DEFINE_HISTOGRAM_STAT specifies the data
many data values fall into each bucket. DEFINE_HISTOGRAM_METRIC specifies the data
Copy link
Contributor Author

@lingbin lingbin Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See:

velox/velox/common/base/StatsReporter.h

Lines 213 to 227 in 2518463

#define DEFINE_HISTOGRAM_METRIC(key, bucket, min, max, ...) \
{ \
if (::facebook::velox::BaseStatsReporter::registered) { \
auto reporter = folly::Singleton< \
facebook::velox::BaseStatsReporter>::try_get_fast(); \
if (FOLLY_LIKELY(reporter != nullptr)) { \
reporter->registerHistogramMetricExportType( \
(key), \
(bucket), \
(min), \
(max), \
(std::vector<int32_t>({__VA_ARGS__}))); \
} \
} \
}

The name of the macro used to register a histogram-type metric is DEFINE_HISTOGRAM_METRIC, not DEFINE_HISTOGRAM_STAT, this PR fixes it.

@lingbin
Fix typo in metrics document
72b6c38 
`DEFINE_HISTOGRAM_STAT` --> `DEFINE_HISTOGRAM_METRIC`

The name of the macro used to register a histogram-type metric is
`DEFINE_HISTOGRAM_METRIC`, not `DEFINE_HISTOGRAM_STAT`, this PR
fixes it.
amitkdutta
amitkdutta approved these changes Jul 14, 2024
View reviewed changes
Copy link
Contributor

@amitkdutta amitkdutta left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @lingbin

@amitkdutta amitkdutta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jul 14, 2024
@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mbasmanova merged this pull request in bf21268.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit bf212687.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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

@amitkdutta amitkdutta amitkdutta approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lingbin @facebook-github-bot @amitkdutta