Skip to content

[native] Add config for stats reporting and counters.#21745

Merged
amitkdutta merged 1 commit intoprestodb:masterfrom
amitkdutta:stats_refactor1
Jan 23, 2024
Merged

[native] Add config for stats reporting and counters.#21745
amitkdutta merged 1 commit intoprestodb:masterfrom
amitkdutta:stats_refactor1

Conversation

@amitkdutta
Copy link
Contributor

Adding config for stats reporting and counter registration. The code is mostly taken from #21599. Separated into a PR as this code can allow to deprecate meta internal logic to enable counters, plus simplify #21599 as well.

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@amitkdutta do we see significant cost from metric collection and report? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

s/enableRuntimeStatsReporting/enableRuntimeMetricReporting/

This is metric but not stats. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

The base class is called BaseStatsReporter and we have defined StatsType enum as well. We seem to use the metric/stata interchangeably throughout.

Copy link
Contributor

Choose a reason for hiding this comment

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

RuntimeStats means the ad hoc added runtime stats. Metric is different: https://facebookincubator.github.io/velox/metrics.html. We should rename BaseStatsReporter to BaseMetricReporter.

@amitkdutta
Copy link
Contributor Author

@amitkdutta do we see significant cost from metric collection and report? Thanks!

@xiaoxmeng Stats reporting is optional and disabled for e2e tests. For meta internal cases, stats reporting starts a thrift service with following code in the PrestoServer wrapper:

  if (!FLAGS_local_mode) {
    facebook::velox::BaseStatsReporter::registered = true;
    facebook::velox::registerFbVeloxCounters();
    facebook::presto::registerFbPrestoCounters();
  }

Now, @karteekmurthys is introducing stats reporting to Premetheus in #21599 where he also needs to set facebook::velox::BaseStatsReporter::registered = true; I think its better to set this in OSS instead of Meta internal wrapper as it unify how stats service is started. Besides, there is another advantage that today inside Meta we start the StatsServer first and then PrestoServer. Ideally we should start PrestoServer, read config and start Stats server if required.

Also since its started from wrapper, there is a flag FLAGS_local_mode to control stat start/stop. This is needed to disable stats during e2e tests. With this config, all these become unified and internally we will overload the function to start stats server.

@jaystarshot
Copy link
Member

jaystarshot commented Jan 22, 2024

I am new to c++ dev, please ignore or consider as nits: Is it possible to add cmake flags instead for people to use so that they can be used in cmake files too?

@karteekmurthys
Copy link
Contributor

I am new to c++ dev, please ignore or consider as nits: Is it possible to add cmake flags instead for people to use so that they can be used in cmake files too?

CMIW, you want COMPILE time control to enable metrics?

@jaystarshot
Copy link
Member

jaystarshot commented Jan 22, 2024

Yes, like ignore compiling a metrics directory if user doesn't want etc and other dependencies

@amitkdutta amitkdutta force-pushed the stats_refactor1 branch 2 times, most recently from e02462f to f565c33 Compare January 22, 2024 23:33
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@amitkdutta LGTM % comments. Thanks!

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@amitkdutta thanks for the iterating!

@amitkdutta amitkdutta merged commit 5a91680 into prestodb:master Jan 23, 2024
@wanglinsong wanglinsong mentioned this pull request Feb 12, 2024
64 tasks
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.

5 participants