[Tracing Instrumentation] Fix for null connection test cases

[Gaganjuneja]

Description

Anomaly-detection has some test cases which passes null connection and those are getting NullPointerException which was earlier handled by the listener. Adding the null connection check in the SpanName builder.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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)
• GitHub issue/PR created in OpenSearch documentation repo for the required public documentation changes (#[Issue/PR number])

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, Please take a look, Its breaking the Anomaly detection plugin's build.

[msfroh]

@Gaganjuneja -- Do you think it makes sense to fix it this way, or should AD just not pass a null connection?

[Gaganjuneja]

@Gaganjuneja -- Do you think it makes sense to fix it this way, or should AD just not pass a null connection?

Ideally yes AD shouldn't pass the null connection but may be there are some test cases which may want to test if their listener::handleException gets called if the connection is null. I guess there is no harm putting up this additional check. What's your thought on this?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

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

Compatibility status:

Checks if related components are compatible with change 17f2952

Incompatible components

Incompatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git]

[Gaganjuneja]

I don't object to the null-check, but on the other hand, part of me likes the idea that the possible NPE highlights other misconfigured things.

💯

Totally agreed, In case needed please feel free to merge this PR :)

[github-actions]

Gradle Check (Jenkins) Run Completed with:

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

[ticheng-aws]

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

[opensearch-trigger-bot]

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

[sohami]

Looking into the code around this area, seems like connection is always expected to be non-null (Ref here). So probably adding a check Objects.nonNull(connection, "Input connection is expected to be non-null" ) in the createSpanName would help to catch such issues (mostly occurring in tests). Currently the PR is ignoring the null connection and expecting the lower layer to handle it or throw the error. Thoughts ?

[reta]

Looking into the code around this area, seems like connection is always expected to be non-null (Ref here).

The connection should never be null indeed

[Gaganjuneja]

Looking into the code around this area, seems like connection is always expected to be non-null (Ref here).

The connection should never be null indeed

+1, This was coming null from test cases.

[dblock]

@Gaganjuneja what do you want to do with this?

If this can't be null maybe the right thing to do is an assert. Otherwise add a test case for the null scenario.

[Gaganjuneja]

I am closing this PR. This is not needed as fixed in the AD plugin.

[dblock]

@Gaganjuneja I think this was a valid case though, being defensive in this code was a good idea. Consider finishing it?

[Gaganjuneja]

Sure, will cleanup the code. Thanks @dblock

[github-actions]

❌ Gradle check result for 17f2952: FAILURE

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.

[mgodwan]

@Gaganjuneja Are you planning to continue on this PR and targeting any upcoming release?
This has been stalled now, and if this is not planned to be continued, lets close this.

[opensearch-trigger-bot]

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