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.

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

[Tracing Instrumentation] Add instrumentation in transport action #10096

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Gaganjuneja
Copy link
Contributor

@Gaganjuneja Gaganjuneja commented Sep 18, 2023

Description

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

Related Issues

Resolves #10095

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • 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
Copy link
Contributor Author

@reta, Please take a look. Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2023

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
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Gagan Juneja added 4 commits September 18, 2023 19:33
Signed-off-by: Gagan Juneja <gjjuneja@amazon.com>
Signed-off-by: Gagan Juneja <gjjuneja@amazon.com>
Signed-off-by: Gagan Juneja <gjjuneja@amazon.com>
Signed-off-by: Gagan Juneja <gjjuneja@amazon.com>
@Gaganjuneja Gaganjuneja force-pushed the main-inst-transport-action branch from 9f3d58c to db2e32e Compare September 18, 2023 14:03
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Sep 19, 2023

@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

@Gaganjuneja
Copy link
Contributor Author

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

@opensearch-trigger-bot
Copy link
Contributor

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

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Oct 21, 2023
@ticheng-aws
Copy link
Contributor

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

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Jan 9, 2024
@opensearch-trigger-bot
Copy link
Contributor

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

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Feb 12, 2024
@sohami
Copy link
Collaborator

sohami commented Feb 14, 2024

@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 ?

@reta
Copy link
Collaborator

reta commented Feb 14, 2024

@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)

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Feb 17, 2024
@opensearch-trigger-bot
Copy link
Contributor

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

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request stalled Issues that have stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tracing Instrumentation] Adds instrumentation at InboundHandler
4 participants