Record source information of HBO stats #22234

feilong-liu · 2024-03-19T00:11:49Z

Description

This PR records more information about HBO stats, including what type of workers (currently c++ and Java) are these stats from, and the query ID which generates these stats.

Motivation and Context

Adding worker type because HBO tracks the size of operator output, however this size can be dependent on the data structure used and compaction algorithm when used. Hence it's expected that presto java and presto c++ can report different size. We need to log this information in HBO stats.

Adding query ID is for debugging purpose. This can help to identify the query which populates the stats quickly.

Impact

Improve on HBO stats to make it more precise and easier to debug.

Test Plan

End to end test locally to make sure these stats are available in logging.

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
* Add worker type and query ID information in HBO stats.

tdcmeehan · 2024-03-19T18:19:47Z

Does it make sense to encode the environment and version? The environment could identify the deployment that wrote the stats (including the eval engine used), and the version could be used to identify any anomalies introduced in the metrics that may vary between version. Similar to how we embed the version in things like Parquet file metadata.

feilong-liu · 2024-03-19T22:36:20Z

Does it make sense to encode the environment and version? The environment could identify the deployment that wrote the stats (including the eval engine used), and the version could be used to identify any anomalies introduced in the metrics that may vary between version. Similar to how we embed the version in things like Parquet file metadata.

Currently I plan to include the type of workers and the query ID of the queries which produce the history statistics. I do not see immediate need of adding environment and version, and they can be inferred from query ID with proper logging. We can add them later if needed, just I do not see immediate need for now.

feilong-liu · 2024-03-21T17:25:54Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsManager.java

    {
        requireNonNull(objectMapper, "objectMapper is null");
        this.sessionPropertyManager = requireNonNull(sessionPropertyManager, "sessionPropertyManager is null");
        this.historyBasedStatisticsCacheManager = new HistoryBasedStatisticsCacheManager();
        ObjectMapper newObjectMapper = objectMapper.copy().configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);
        this.planCanonicalInfoProvider = new CachingPlanCanonicalInfoProvider(historyBasedStatisticsCacheManager, newObjectMapper, metadata);
        this.config = requireNonNull(config, "config is null");
+        this.isNativeExecution = featuresConfig.isNativeExecutionEnabled();


Check if it's native execution, and record the information when writing stats to HBO

feilong-liu · 2024-03-21T17:27:42Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsCalculator.java

+                            if (predictedPlanStatistics.getConfidence() > 0) {
+                                return delegateStats.combineStats(
+                                        predictedPlanStatistics,
+                                        new HistoryBasedSourceInfo(entry.getKey().getHash(), inputTableStatistics, Optional.of(historicalPlanStatisticsEntry.get().getHistoricalPlanStatisticsEntryInfo())));


Add the source information to plan statistics returned from HBO.

feilong-liu · 2024-03-21T17:59:24Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsTracker.java

+        HistoricalPlanStatisticsEntryInfo historicalPlanStatisticsEntryInfo = new HistoricalPlanStatisticsEntryInfo(
+                isNativeExecution ? HistoricalPlanStatisticsEntryInfo.WorkerType.CPP : HistoricalPlanStatisticsEntryInfo.WorkerType.JAVA, queryInfo.getQueryId());
+


record the worker type and query ID when recording the HBO stats

Can we get this isNativeExecution property via session directly? queryInfo.getSession().toSession(sessionPropertyManager); That way you wound not need to inject featuresConfig

@jaystarshot Jay, we explicitly removed this property from the sessions because it doesn't make sense to allow this to be modified for individual queries. This is a cluster-wide property. I have a PR to actually delete it: #22183

CC: @tdcmeehan

rschlussel · 2024-03-21T19:13:56Z

I like the idea of recording the version. That way if there's a problem with stats from some release or some big change in an operator that would make old stats not relevant, we could programatically exclude those stats from being used.

feilong-liu · 2024-03-22T17:50:39Z

presto-main/src/main/java/com/facebook/presto/cost/HistoryBasedPlanStatisticsManager.java

    {
        requireNonNull(objectMapper, "objectMapper is null");
        this.sessionPropertyManager = requireNonNull(sessionPropertyManager, "sessionPropertyManager is null");
        this.historyBasedStatisticsCacheManager = new HistoryBasedStatisticsCacheManager();
        ObjectMapper newObjectMapper = objectMapper.copy().configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true);
        this.planCanonicalInfoProvider = new CachingPlanCanonicalInfoProvider(historyBasedStatisticsCacheManager, newObjectMapper, metadata);
        this.config = requireNonNull(config, "config is null");
+        this.isNativeExecution = featuresConfig.isNativeExecutionEnabled();
+        this.serverVersion = requireNonNull(nodeVersion, "nodeVersion is null").toString();


Add information for server version

feilong-liu · 2024-03-22T17:51:02Z

I like the idea of recording the version. That way if there's a problem with stats from some release or some big change in an operator that would make old stats not relevant, we could programatically exclude those stats from being used.

Add server version per suggestion.

...-spi/src/main/java/com/facebook/presto/spi/statistics/HistoricalPlanStatisticsEntryInfo.java

Record the type of workers (CPP, JAVA) and query ID of the queries which produce these stats.

feilong-liu requested review from shrinidhijoshi and a team as code owners March 19, 2024 00:11

feilong-liu requested a review from presto-oss March 19, 2024 00:11

feilong-liu marked this pull request as draft March 19, 2024 00:11

feilong-liu force-pushed the hbo_cpp_label branch from 7f1e575 to b576187 Compare March 19, 2024 17:41

feilong-liu force-pushed the hbo_cpp_label branch 2 times, most recently from 3c1aa54 to 6de650a Compare March 19, 2024 22:03

feilong-liu changed the title ~~Record if HBO stats is from CPP or JAVA~~ Record source information of HBO stats Mar 19, 2024

feilong-liu force-pushed the hbo_cpp_label branch 3 times, most recently from 82abb6d to 01f9257 Compare March 21, 2024 17:57

feilong-liu commented Mar 21, 2024

View reviewed changes

feilong-liu marked this pull request as ready for review March 21, 2024 18:31

feilong-liu requested review from pranjalssh, jaystarshot, rschlussel and mlyublena March 21, 2024 18:31

feilong-liu force-pushed the hbo_cpp_label branch from 01f9257 to 8df83cc Compare March 22, 2024 17:49

feilong-liu commented Mar 22, 2024

View reviewed changes

feilong-liu force-pushed the hbo_cpp_label branch from 8df83cc to 16b95c8 Compare March 22, 2024 18:57

pranjalssh previously approved these changes Mar 25, 2024

View reviewed changes

...-spi/src/main/java/com/facebook/presto/spi/statistics/HistoricalPlanStatisticsEntryInfo.java Outdated Show resolved Hide resolved

feilong-liu dismissed pranjalssh’s stale review via 43593e1 March 25, 2024 23:26

feilong-liu force-pushed the hbo_cpp_label branch from 16b95c8 to 43593e1 Compare March 25, 2024 23:26

feilong-liu requested a review from pranjalssh March 25, 2024 23:26

Record source information for HBO stats

3aa03b5

Record the type of workers (CPP, JAVA) and query ID of the queries which produce these stats.

feilong-liu force-pushed the hbo_cpp_label branch from 43593e1 to 3aa03b5 Compare March 26, 2024 05:05

pranjalssh approved these changes Mar 26, 2024

View reviewed changes

feilong-liu merged commit 8b8c9e4 into prestodb:master Mar 26, 2024
56 checks passed

feilong-liu deleted the hbo_cpp_label branch March 26, 2024 19:42

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Record source information of HBO stats #22234

Record source information of HBO stats #22234

feilong-liu commented Mar 19, 2024 •

edited

Loading

tdcmeehan commented Mar 19, 2024

feilong-liu commented Mar 19, 2024

feilong-liu Mar 21, 2024

feilong-liu Mar 21, 2024

feilong-liu Mar 21, 2024

jaystarshot Mar 21, 2024

mbasmanova Mar 21, 2024

rschlussel commented Mar 21, 2024

feilong-liu Mar 22, 2024

feilong-liu commented Mar 22, 2024

		HistoricalPlanStatisticsEntryInfo historicalPlanStatisticsEntryInfo = new HistoricalPlanStatisticsEntryInfo(
		isNativeExecution ? HistoricalPlanStatisticsEntryInfo.WorkerType.CPP : HistoricalPlanStatisticsEntryInfo.WorkerType.JAVA, queryInfo.getQueryId());

Record source information of HBO stats #22234

Record source information of HBO stats #22234

Conversation

feilong-liu commented Mar 19, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

tdcmeehan commented Mar 19, 2024

feilong-liu commented Mar 19, 2024

feilong-liu Mar 21, 2024

Choose a reason for hiding this comment

feilong-liu Mar 21, 2024

Choose a reason for hiding this comment

feilong-liu Mar 21, 2024

Choose a reason for hiding this comment

jaystarshot Mar 21, 2024

Choose a reason for hiding this comment

mbasmanova Mar 21, 2024

Choose a reason for hiding this comment

rschlussel commented Mar 21, 2024

feilong-liu Mar 22, 2024

Choose a reason for hiding this comment

feilong-liu commented Mar 22, 2024

feilong-liu commented Mar 19, 2024 •

edited

Loading