OpenSearch icon indicating copy to clipboard operation
OpenSearch copied to clipboard

[Tracing Instrumentation] Add instrumentation in transport action

Open Gaganjuneja opened this issue 1 year ago • 13 comments

Description

Add instrumentation in TransportAction class so that it's generically available to all the actions.

Related Issues

Resolves #10095

Check List

  • [x] New functionality includes testing.
    • [x] All tests pass
  • [x] New functionality has been documented.
    • [x] New functionality has javadoc added
  • [x] Commits are signed per the DCO using --signoff
  • [x] Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Gaganjuneja avatar Sep 18 '23 12:09 Gaganjuneja

@reta, Please take a look. Thanks!

Gaganjuneja avatar Sep 18 '23 12:09 Gaganjuneja

Compatibility status:

Checks if related components are compatible with change db2e32e

Incompatible components

Incompatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/reporting.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/performance-analyzer-rca.git]

github-actions[bot] avatar Sep 18 '23 12:09 github-actions[bot]

Gradle Check (Jenkins) Run Completed with:

  • RESULT: FAILURE :x:
  • URL: https://build.ci.opensearch.org/job/gradle-check/25771/
  • CommitID: 3df1593d24d16d4c6baa2cc2bde9d0f463e59a7d Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions[bot] avatar Sep 18 '23 12:09 github-actions[bot]

Gradle Check (Jenkins) Run Completed with:

  • RESULT: FAILURE :x:
  • URL: https://build.ci.opensearch.org/job/gradle-check/25772/
  • CommitID: 325a8f2b9709c63489689efc48387604b9e2079b Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions[bot] avatar Sep 18 '23 12:09 github-actions[bot]

Gradle Check (Jenkins) Run Completed with:

  • RESULT: FAILURE :x:
  • URL: https://build.ci.opensearch.org/job/gradle-check/25774/
  • CommitID: 9f3d58c46dfde5e690c986d814b51a5c4f033f5e Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions[bot] avatar Sep 18 '23 13:09 github-actions[bot]

Gradle Check (Jenkins) Run Completed with:

  • RESULT: FAILURE :x:
  • URL: https://build.ci.opensearch.org/job/gradle-check/25782/
  • CommitID: db2e32ec212483614b649ac9a80c242fc17c019a Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions[bot] avatar Sep 18 '23 14:09 github-actions[bot]

@reta, Please take a look. Thanks!

@Gaganjuneja I honestly doubt that this is what we should be doing:

  • this change will break each and every plugin (since it changes the TransportAction base class)
  • more importantly, transport actions are always executed in context of remote (or local) request, there is no need to instrument each but only the one place when they are materialized and executed

As with HTTP actions, we have to take a step back and start from the beginning, the network layer, which is denoted by InboundHandler

reta avatar Sep 19 '23 20:09 reta

@reta, Please take a look. Thanks!

@Gaganjuneja I honestly doubt that this is what we should be doing:

  • this change will break each and every plugin (since it changes the TransportAction base class)
  • more importantly, transport actions are always executed in context of remote (or local) request, there is no need to instrument each but only the one place when they are materialized and executed

As with HTTP actions, we have to take a step back and start from the beginning, the network layer, which is denoted by InboundHandler

@reta, I agree, We should be instrumenting the specific implementation of a TransportAction based on the requirement and that would add more value, This I realized when I was looking at the TransportBulkAction. I have raised another PR (#10143) for InboundHandler. One catch here is that it might break again the security plugin.

Gaganjuneja avatar Sep 20 '23 17:09 Gaganjuneja

This PR is stalled because it has been open for 30 days with no activity.

Hi @Gaganjuneja, the PR is stalled. Is this being worked upon? Feel free to reach out to maintainers for further reviews.

ticheng-aws avatar Jan 06 '24 00:01 ticheng-aws

This PR is stalled because it has been open for 30 days with no activity.

@Gaganjuneja @reta Do we still plan to iterate on this PR (considering this is introducing a breaking change ?) If not, then can we consider closing this PR out ?

sohami avatar Feb 14 '24 20:02 sohami

@Gaganjuneja @reta Do we still plan to iterate on this PR (considering this is introducing a breaking change ?) If not, then can we consider closing this PR out ?

@Gaganjuneja certainly your call, I think the breaking part should not be an issue (in this particular case)

reta avatar Feb 14 '24 23:02 reta

This PR is stalled because it has been open for 30 days with no activity.