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

Initial integration of TelemetryAwarePlugin with Rollover Step Integration #1186

Closed
wants to merge 4 commits into from
Closed

Initial integration of TelemetryAwarePlugin with Rollover Step Integration #1186

wants to merge 4 commits into from

Conversation

harshitakaushik-dev
Copy link
Contributor

Description of changes:

  • Initialised TelemetryAwarePlugin in IndexManagementPlugin.kt
  • Created IndexManagementActionsMetrics.kt which defines a singleton class IndexManagementActionsMetrics that provides metrics for different OpenSearch Index Management actions (like rollover) by registering them with a metrics registry.

CheckList:

  • 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.

Initial integration of TelemetryAwarePlugin to ISM
83d53d5 
Signed-off-by: harycash <harycash@amazon.com>
sarthakaggarwal97
sarthakaggarwal97 reviewed Jun 11, 2024
View reviewed changes
src/main/kotlin/org/opensearch/indexmanagement/IndexManagementPlugin.kt Outdated
import org.opensearch.indexmanagement.indexstatemanagement.ManagedIndexRunner
import org.opensearch.indexmanagement.indexstatemanagement.PluginVersionSweepCoordinator
import org.opensearch.indexmanagement.indexstatemanagement.SkipExecution
import org.opensearch.indexmanagement.indexstatemanagement.*
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not use wildcard

sarthakaggarwal97
sarthakaggarwal97 reviewed Jun 11, 2024
View reviewed changes
src/main/kotlin/org/opensearch/indexmanagement/IndexManagementPlugin.kt Outdated
import org.opensearch.plugins.NetworkPlugin
import org.opensearch.plugins.Plugin
import org.opensearch.plugins.SystemIndexPlugin
import org.opensearch.plugins.*
Copy link
Contributor

Choose a reason for hiding this comment

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

same, please remove wildcard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've made the updates in the new commit.

sarthakaggarwal97
sarthakaggarwal97 reviewed Jun 11, 2024
View reviewed changes
.../kotlin/org/opensearch/indexmanagement/indexstatemanagement/IndexManagementActionsMetrics.kt Outdated
fun initialize(metricsRegistry: MetricsRegistry) {
this.metricsRegistry = metricsRegistry
actionMetricsMap = mapOf(
"rollover" to RolloverActionMetrics(metricsRegistry),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not defined yet, please remove this for now. We can populate the map in the subsequent PR

Initial integration of TelemetryAwarePlugin to ISM
681d015 
Signed-off-by: harycash <harycash@amazon.com>
@sarthakaggarwal97
Copy link
Contributor

@harshitakaushik-dev thanks for iteration, would you please add some UTs to verify the behavior in the next iteration. Also, please check some lint related gradle tasks seem to be failing.

Initial integration of TelemetryAwarePlugin to ISM with Rollover Step…
c3b2846 
… Integration

Signed-off-by: harycash <harycash@amazon.com>
@harshitakaushik-dev harshitakaushik-dev changed the title Initial integration of TelemetryAwarePlugin to ISM Initial integration of TelemetryAwarePlugin with Rollover Step Integration Jun 13, 2024
@sarthakaggarwal97
Copy link
Contributor

@bowenlan-amzn @Gaganjuneja it would be great if you guys can take a look!

The file diff id huge, but its mostly change in arguments of a function. The change for one action would somewhat look like this commit

Initial integration of TelemetryAwarePlugin to ISM with Rollover Step…
9eaf83a 
… Integration

Signed-off-by: harycash <harycash@amazon.com>
@bowenlan-amzn
Copy link
Member

@sarthakaggarwal97 commented on Jun 12, 2024, 10:23 PM PDT:

@bowenlan-amzn @Gaganjuneja it would be great if you guys can take a look!

The file diff id huge, but its mostly change in arguments of a function. The change for one action would somewhat look like this commit

Originally posted by @sarthakaggarwal97 in #1186 (comment)

Really like this change, with metrics, user can setup alarm instead of only relying on the error notifications, which is a long-asking feature.
I will go through this later. Before that, can we have a GitHub issue for this that's public facing and later on copied to documentation.

r1walz
r1walz requested changes Jun 18, 2024
View reviewed changes
Copy link
Collaborator

@r1walz r1walz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for PR @harshitakaushik-dev! Commenting from a quick look and it's a good start. Can you please look into the check failures and fix the cause. I'm interested in reviewing in depth later. Thanks!

...opensearch.indexmanagement.spi/indexstatemanagement/metrics/IndexManagementActionsMetrics.kt Show resolved Hide resolved
...opensearch.indexmanagement.spi/indexstatemanagement/metrics/IndexManagementActionsMetrics.kt Show resolved Hide resolved
...opensearch.indexmanagement.spi/indexstatemanagement/metrics/IndexManagementActionsMetrics.kt Show resolved Hide resolved
...lin/org/opensearch/indexmanagement/indexstatemanagement/step/rollover/AttemptRolloverStep.kt
val context = this.context ?: return this
val indexName = context.metadata.index
val clusterService = context.clusterService
val skipRollover = clusterService.state().metadata.index(indexName).getRolloverSkip()
this.indexManagementActionsMetrics = indexManagementActionsMetrics
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we use it after assignment? What's the need here?

...lin/org/opensearch/indexmanagement/indexstatemanagement/step/rollover/AttemptRolloverStep.kt
val context = this.context ?: return this
val indexName = context.metadata.index
val clusterService = context.clusterService
val skipRollover = clusterService.state().metadata.index(indexName).getRolloverSkip()
this.indexManagementActionsMetrics = indexManagementActionsMetrics
this.actionMetrics = indexManagementActionsMetrics.getActionMetrics("rollover") as RolloverActionMetrics
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use "rollover" as const.

...lin/org/opensearch/indexmanagement/indexstatemanagement/step/rollover/AttemptRolloverStep.kt
Comment on lines +80 to +82
Tags.create().addTag("index_name", context.metadata.index)
.addTag("policy_id", context.metadata.policyID).addTag("node_id", context.clusterService.nodeName),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be encapsulated inside of RolloverActionMetrics/ActionMetrics itself?

...lin/org/opensearch/indexmanagement/indexstatemanagement/step/rollover/AttemptRolloverStep.kt

@Suppress("ComplexMethod", "LongMethod")
override suspend fun execute(): Step {
override suspend fun execute(indexManagementActionsMetrics: IndexManagementActionsMetrics): Step {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Corresponding param in super class (Step) doesn't match the param name here. It will result in compilation failures.

@harshitakaushik-dev
Copy link
Contributor Author

@r1walz Made the requested changes in the new PR with additional action metrics. Request you to kindly review PR #1195

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

@sarthakaggarwal97 sarthakaggarwal97 sarthakaggarwal97 left review comments

@r1walz r1walz r1walz requested changes

@bowenlan-amzn bowenlan-amzn Awaiting requested review from bowenlan-amzn bowenlan-amzn is a code owner

@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02

@lezzago lezzago Awaiting requested review from lezzago

@praveensameneni praveensameneni Awaiting requested review from praveensameneni

@xluo-aws xluo-aws Awaiting requested review from xluo-aws

@gaobinlong gaobinlong Awaiting requested review from gaobinlong

@Hailong-am Hailong-am Awaiting requested review from Hailong-am Hailong-am is a code owner

@amsiglan amsiglan Awaiting requested review from amsiglan

@sbcd90 sbcd90 Awaiting requested review from sbcd90

@eirsep eirsep Awaiting requested review from eirsep

@AWSHurneyt AWSHurneyt Awaiting requested review from AWSHurneyt

@jowg-amazon jowg-amazon Awaiting requested review from jowg-amazon

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

Assignees

@harshitakaushik-dev harshitakaushik-dev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harshitakaushik-dev @sarthakaggarwal97 @bowenlan-amzn @r1walz