Skip to content

feat: expose comet metrics through Sparks external monitoring system#3708

Open
coderfender wants to merge 4 commits intoapache:mainfrom
coderfender:project_comet_metrics
Open

feat: expose comet metrics through Sparks external monitoring system#3708
coderfender wants to merge 4 commits intoapache:mainfrom
coderfender:project_comet_metrics

Conversation

@coderfender
Copy link
Contributor

@coderfender coderfender commented Mar 15, 2026 •
edited
Loading

Which issue does this PR close?

Closes #3712

Rationale for this change

  1. We currently collect metrics and show while logging in comet. This PR is to essentially go a step further and wire the metrics out to be visualized in comet through tools like Grafana . As more users start using comet, these metrics would be helpful in understanding acceleration from comet standpoint. These are first Comet metrics exposed to Spark's external monitoring system and more would follow

The following metrics are exposed at the moment :

  1. comet.operators.native
  2. comet.operators.spark
  3. comet.queries.planned
  4. comet.transitions
  5. comet.acceleration.ratio (acceleration : native / (native + spark)

What changes are included in this PR?

How are these changes tested?

@coderfender coderfender force-pushed the project_comet_metrics branch from aa97dec to f096d8f Compare March 15, 2026 22:02
@coderfender
Copy link
Contributor Author

coderfender commented Mar 15, 2026

cc : @andygrove

@coderfender coderfender changed the title feat: expose comet metrics for visualization feat: expose comet metrics through Sparks external monitoring system Mar 16, 2026
wForget
wForget reviewed Mar 16, 2026
View reviewed changes
@coderfender coderfender force-pushed the project_comet_metrics branch from f096d8f to c0eb149 Compare March 16, 2026 03:03
@coderfender
Copy link
Contributor Author

coderfender commented Mar 16, 2026

@wForget , Please take a look whenever you get a chance

@coderfender coderfender force-pushed the project_comet_metrics branch from c0eb149 to 2eb506f Compare March 16, 2026 15:52
wForget
wForget reviewed Mar 17, 2026
View reviewed changes
@coderfender coderfender force-pushed the project_comet_metrics branch from 8c60dc8 to eaa1dc4 Compare March 17, 2026 08:10
@coderfender
Copy link
Contributor Author

coderfender commented Mar 17, 2026

@wForget , I addressed code per your review comments. Please take a look whenever you get a chance. Thank you for the kind guidance

wForget
wForget reviewed Mar 17, 2026
View reviewed changes
spark/src/test/scala/org/apache/spark/CometPluginsSuite.scala Outdated
Comment on lines +106 to +107
val queriesBefore = CometSource.QUERIES_PLANNED.getCount
spark.sparkContext.listenerBus.waitUntilEmpty()
Copy link
Member

@wForget wForget Mar 17, 2026

Choose a reason for hiding this comment

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

Suggested change
val queriesBefore = CometSource.QUERIES_PLANNED.getCount
spark.sparkContext.listenerBus.waitUntilEmpty()
spark.sparkContext.listenerBus.waitUntilEmpty()
val queriesBefore = CometSource.QUERIES_PLANNED.getCount

wForget
wForget reviewed Mar 17, 2026
View reviewed changes
spark/src/main/scala/org/apache/spark/Plugins.scala Outdated
}

object CometDriverPlugin extends Logging {
def registerQueryExecutionListener(conf: SparkConf): Unit = {
Copy link
Member

@wForget wForget Mar 17, 2026

Choose a reason for hiding this comment

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

  1. move sc.env.metricsSystem.registerSource(CometSource) into this method, and add a configuration to make this behavior configurable
  2. Check if spark.sql.queryExecutionListeners is configured and merge it. You can refer to the implementation of registerCometSessionExtension method.

Copy link
Contributor

@parthchandra parthchandra Mar 17, 2026

Choose a reason for hiding this comment

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

+1. Current implementation overrides any user define listeners.

parthchandra
parthchandra reviewed Mar 17, 2026
View reviewed changes
spark/src/main/scala/org/apache/spark/Plugins.scala Outdated
}

object CometDriverPlugin extends Logging {
def registerQueryExecutionListener(conf: SparkConf): Unit = {
Copy link
Contributor

@parthchandra parthchandra Mar 17, 2026

Choose a reason for hiding this comment

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

+1. Current implementation overrides any user define listeners.

spark/src/main/scala/org/apache/spark/CometSource.scala
}
})

def recordStats(stats: CometCoverageStats): Unit = {
Copy link
Contributor

@parthchandra parthchandra Mar 17, 2026

Choose a reason for hiding this comment

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

This isn't exactly ideal. These stats make sense per plan. Here we are incrementing all metrics globally, so all plans that are executed across all sessions in a Spark application will have their counts accumulated and may provide no useful information.

Copy link
Contributor Author

@coderfender coderfender Mar 18, 2026

Choose a reason for hiding this comment

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

Sure. But spark does follow a similar approach here but there is always an option to improve granularity

@coderfender
Copy link
Contributor Author

coderfender commented Mar 18, 2026

Thwnk you for the review . Let me take a look and update the logic

@coderfender coderfender force-pushed the project_comet_metrics branch from c6da902 to 2b6dcc1 Compare March 18, 2026 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@parthchandra parthchandra parthchandra left review comments

@wForget wForget wForget left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Export comet metrics externally through spark's monitoring system

3 participants

@coderfender @parthchandra @wForget