Adds instrumentation for search path

[Gaganjuneja]

Description

Adds the instrumentation on Search path. Making it a Draft PR to discuss the instrumentation on SearchService.java whether this is the right way to do explicit tracing or should we do inside SearchOperationsListener, which will require the state management.

Related Issues

Resolves #[7546]

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]

@reta @shwetathareja Please provide your thoughts on this.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/18955/
• CommitID: 7cd788f
  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]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/19143/
• CommitID: a4d3ba8
  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]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      2 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testNodeDropWithOngoingReplication
      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testPrimaryRelocation

• URL: https://build.ci.opensearch.org/job/gradle-check/19142/
• CommitID: a4d3ba8
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[reta]

By flag you mean levels or some component level settings?

Nope, see please [1]

Trace Flags, a binary encoding containing information about the trace

[1] https://opentelemetry.io/docs/concepts/signals/traces/#span-context

[Gaganjuneja]

@reta , I am working on documentation task for tracing framework features for release 2.9. Which includes how somebody can enable/disable the feature and other settings. While I fully appreciate and support the holistic approach you've outlined for adding instrumentation, I believe it would be beneficial to include a basic search request instrumentation as a stepping stone that users can easily relate to. (SearchTask -> QueryPhase -> FetchPhase)

I have created a meta task for the overall tracing implementation and will continue working on that. I tried doing the changes for TransportAction but it's a lot of work and requires testing as there are hundreds of implementations of TransportAction and we need to pass the TracingFactory instance from all concrete classes.

[reta]

While I fully appreciate and support the holistic approach you've outlined for adding instrumentation, I believe it would be beneficial to include a basic search request instrumentation as a stepping stone that users can easily relate to. (SearchTask -> QueryPhase -> FetchPhase)

As I mentioned, I don't see the point in rushing the instrumentation in the current form (it could be replaced by a simple log statements if there is an urgent need to do so). If others see the value in it - I have no objections.

[Bukhtawar]

Thanks @Gaganjuneja for this, wanted to understand more around the changes. As I see, the tracing is being done mostly around the shard level search execution SearchServices as long as changes are pertaining to this class or specific search operations, without touching the distributed transport layers and task coordinations I would still be fine with it since highly likely they won't change. But the moment you change the transport layers TransportSearchAction it makes me concerned if we are writing it generic enough and thinking through how the framework interactions would evolve.
If you can isolate core search module instrumentations and still use this PR to build search shard level interactions more generically, I am good.

The benefit I see with doing this way is, if there are core deficiencies in the tracing framework we have built so far, it would help us understand those gaps upfront and course correct without having to invest into into a framework first and then thinking how best these core modules would fit in

@reta what do you think?

[reta]

Thanks @Bukhtawar

@reta what do you think?

We already found one with Tracing API [1], this is good (in my opinion) and we could address that now.

But the moment you change the transport layers TransportSearchAction it makes me concerned if we are writing it generic enough and thinking through how the framework interactions would evolve.

That the most complicated and arguably the most important part of the work. It opens up the way for context transfer and actually getting real benefits of distributed nature of the tracing (not only for core but beyond that). What we get with this (isolated) instrumentation - out of the blue uncorrelated traces. This is what logs are for (my opinion).

[shwetathareja]

@Gaganjuneja @Bukhtawar
I completely agree to that point that we should have more sample instrumentation across different code paths to see if there are gaps with the tracing framework we have built so far. But agree with @reta here that thinking on how we are going to introduce the tracing at the transport level generically is important as without that others might not be able to use it holistically.
This PR does start the discussion on one other aspect whether we should simply do inline instrumentation vs use the current listeners SearchOperationListener and what changes we need for the same.

[Gaganjuneja]

@Gaganjuneja @Bukhtawar I completely agree to that point that we should have more sample instrumentation across different code paths to see if there are gaps with the tracing framework we have built so far. But agree with @reta here that thinking on how we are going to introduce the tracing at the transport level generically is important as without that others might not be able to use it holistically. This PR does start the discussion on one other aspect whether we should simply do inline instrumentation vs use the current listeners SearchOperationListener and what changes we need for the same.

Absolutely, @shwetathareja, We need to decide on that as well, explicit inline instrumentation looks good to me.

[Gaganjuneja]

Thanks @Bukhtawar

@reta what do you think?

We already found one with Tracing API [1], this is good (in my opinion) and we could address that now.

But the moment you change the transport layers TransportSearchAction it makes me concerned if we are writing it generic enough and thinking through how the framework interactions would evolve.

That the most complicated and arguably the most important part of the work. It opens up the way for context transfer and actually getting real benefits of distributed nature of the tracing (not only for core but beyond that). What we get with this (isolated) instrumentation - out of the blue uncorrelated traces. This is what logs are for (my opinion).

@reta @Bukhtawar Please take a look once. I have added the instrumentation in TransportAction directly and removed from the TransportSearchAction.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/19569/
• CommitID: 479a937
  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?

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity. Remove stalled label or comment or this will be closed in 7 days.

[opensearch-trigger-bot]

This PR was closed because it has been stalled for 7 days with no activity.

[kotwanikunal]

Apologies. This PR was auto closed without reaching a resolution from the maintainers.
Re-opening to move it forward.
Thanks for your contributions to OpenSearch!

[github-actions]

Compatibility status:

Checks if related components are compatible with change 479a937

Incompatible components

Skipped components

Compatible components

Compatible components: []

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/25619/
• CommitID: 479a937
  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?

[opensearch-trigger-bot]

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

[kotwanikunal]

There has been no movement on this PR. @Gaganjuneja Please reopen if you still plan to work on it.