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

Add external scheduler metrics #827

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Add external scheduler metrics #827

merged 3 commits into from
Oct 31, 2024

Conversation

ykmr1224
Copy link
Collaborator

@ykmr1224 ykmr1224 commented Oct 28, 2024 •
edited
Loading

Description

  • Add external scheduler metrics
    • externalScheduler.{action}.count

Tested in my dev environment:
externalScheduler metrics

Related Issues

n/a

Check List

  • Updated documentation (docs/ppl-lang/README.md)
  • Implemented unit tests
  • Implemented tests for combination with other commands
  • New added source code should include a copyright header
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ykmr1224
Add external scheduler metrics
e1a22f5 
Signed-off-by: Tomoyuki Morita <[email protected]>
@ykmr1224
Format code
8390b2a 
Signed-off-by: Tomoyuki Morita <[email protected]>
noCharger
noCharger reviewed Oct 29, 2024
View reviewed changes
Copy link
Collaborator

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

these already exist in flint-spark-integration/src/main/scala/org/opensearch/flint/spark/refresh/util/RefreshMetricsHelper.scala

@ykmr1224
Copy link
Collaborator Author

these already exist in flint-spark-integration/src/main/scala/org/opensearch/flint/spark/refresh/util/RefreshMetricsHelper.scala

It is intended to track each action. Is the existing metric track them? (I though it is tracking success/failure/latency for exernal scheduler action)

@noCharger
Copy link
Collaborator

these already exist in flint-spark-integration/src/main/scala/org/opensearch/flint/spark/refresh/util/RefreshMetricsHelper.scala

It is intended to track each action. Is the existing metric track them? (I though it is tracking success/failure/latency for exernal scheduler action)

what's the benefit of adding action? it's mainly opensearch CRUD

@ykmr1224 ykmr1224 marked this pull request as ready for review October 29, 2024 05:20
@ykmr1224
Copy link
Collaborator Author

these already exist in flint-spark-integration/src/main/scala/org/opensearch/flint/spark/refresh/util/RefreshMetricsHelper.scala

It is intended to track each action. Is the existing metric track them? (I though it is tracking success/failure/latency for exernal scheduler action)

what's the benefit of adding action? it's mainly opensearch CRUD

It would show how often user add/modify/delete manual refresh. If we can easily know it with other metrics, we can skip it.

noCharger
noCharger reviewed Oct 30, 2024
View reviewed changes
...main/scala/org/opensearch/flint/spark/scheduler/FlintSparkJobExternalSchedulingService.scala
val actionName = action.name().toLowerCase()
MetricsUtil.addHistoricGauge(
MetricConstants.EXTERNAL_SCHEDULER_METRIC_PREFIX + actionName + ".count",
1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will HistoricGauge with value one aggregated on client side?

...main/scala/org/opensearch/flint/spark/scheduler/FlintSparkJobExternalSchedulingService.scala
@@ -83,8 +84,16 @@ class FlintSparkJobExternalSchedulingService(
case AsyncQuerySchedulerAction.REMOVE => flintAsyncQueryScheduler.removeJob(request)
case _ => throw new IllegalArgumentException(s"Unsupported action: $action")
}
addExternalSchedulerMetrics(action)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we reuse withMetrics by adding the action in prefix?

@ykmr1224 ykmr1224 merged commit c590d29 into opensearch-project:main Oct 31, 2024
4 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 31, 2024
@github-actions
Add external scheduler metrics (#827)
98a09cb 
* Add external scheduler metrics

Signed-off-by: Tomoyuki Morita <[email protected]>

* Format code

Signed-off-by: Tomoyuki Morita <[email protected]>

---------

Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki MORITA <[email protected]>
(cherry picked from commit c590d29)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 31, 2024
Merged
ykmr1224 pushed a commit that referenced this pull request Oct 31, 2024
@opensearch-trigger-bot @github-actions
Add external scheduler metrics (#827) (#856)
c190f01 
* Add external scheduler metrics



* Format code



---------



(cherry picked from commit c590d29)

Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki MORITA <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@ykmr1224 ykmr1224 deleted the metrics/external_scheduler branch October 31, 2024 05:06
kenrickyap pushed a commit to Bit-Quill/opensearch-spark that referenced this pull request Dec 11, 2024
@ykmr1224 @14yapkc1
Add external scheduler metrics (opensearch-project#827)
c995ccf 
* Add external scheduler metrics

Signed-off-by: Tomoyuki Morita <[email protected]>

* Format code

Signed-off-by: Tomoyuki Morita <[email protected]>

---------

Signed-off-by: Tomoyuki Morita <[email protected]>
Signed-off-by: Tomoyuki MORITA <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noCharger noCharger noCharger approved these changes

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@rupal-bq rupal-bq Awaiting requested review from rupal-bq

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

@vmmusings vmmusings Awaiting requested review from vmmusings

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

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

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

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

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

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

Assignees
No one assigned
Labels
backport 0.5-nexus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ykmr1224 @noCharger