feat(detector-instana)!: change implementation to DetectorSync interface

[david-luna]

Context

Another PR moving away from the deprecated Detector interface in order to prepare the codebase for a fix for #2320. This time is for Instana detector.

My reasons to categorize it as a refactor are:

• SDK handles both interfaces using detectResources from @opentelemetry/resources package. Ref
• it is unlikely to be used oustide the SDK or auto-instrumentations

Short description of the changes

• change detect method to be sync
• update tests

Test changes

Previously the test where expecting the detector to throw but with the new implementation is not possible throw in a synchornous detect method when an async HTTP request fails. The behaviour has been changed to return an empty resource instead. This change is compatible with the current SDK since it actually does the same logic when a detector fails. You can check the detectResources implementation link above.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.68%. Comparing base (dfb2dff) to head (cde57e3).
Report is 233 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2337      +/-   ##
==========================================
- Coverage   90.97%   90.68%   -0.30%     
==========================================
  Files         146      156      +10     
  Lines        7492     7632     +140     
  Branches     1502     1575      +73     
==========================================
+ Hits         6816     6921     +105     
- Misses        676      711      +35     

Files with missing lines	Coverage Δ
...ctor-instana/src/detectors/InstanaAgentDetector.ts	96.66% <100.00%> (+0.23%)	⬆️

... and 76 files with indirect coverage changes

[blumamir]

LGTM, but I wonder if we should still mark it as breaking just in case.

[david-luna]

@basti1302 @kirrg001 is it okay for you to have this breaking change? If not we I can instead add a new detector implementing the sync interface

[basti1302]

Hi David, I do no longer work for IBM/Instana, so I don't have any particular opinion about this.

[david-luna]

Thanks @basti1302. Then we will go for the breaking change

[blumamir]

As discussed in the last SIG and documented on the PR, it is very unlikely that people out there are calling the InstanaAgentDetector directly and will be affected by changing the implements Detector to implements DetectorSync.

Since the package is still experimental (major version 0), we can make this change as breaking.

@open-telemetry/javascript-approvers any objections before I merge this PR?

[pichlermarc]

As discussed in the last SIG and documented on the PR, it is very unlikely that people out there are calling the InstanaAgentDetector directly and will be affected by changing the implements Detector to implements DetectorSync.

Since the package is still experimental (major version 0), we can make this change as breaking.

@open-telemetry/javascript-approvers any objections before I merge this PR?

I'm mainly waiting for someone from Instana to comment. Maybe we give it a couple more days until we merge this to ensure they have had enough time to comment.

[blumamir]

As discussed in the last SIG and documented on the PR, it is very unlikely that people out there are calling the InstanaAgentDetector directly and will be affected by changing the implements Detector to implements DetectorSync.
Since the package is still experimental (major version 0), we can make this change as breaking.
@open-telemetry/javascript-approvers any objections before I merge this PR?

I'm mainly waiting for someone from Instana to comment. Maybe we give it a couple more days until we merge this to ensure they have had enough time to comment.

@kirrg001 can you please take a look?

[david-luna]

As discussed in the last SIG and documented on the PR, it is very unlikely that people out there are calling the InstanaAgentDetector directly and will be affected by changing the implements Detector to implements DetectorSync.
Since the package is still experimental (major version 0), we can make this change as breaking.
@open-telemetry/javascript-approvers any objections before I merge this PR?

I'm mainly waiting for someone from Instana to comment. Maybe we give it a couple more days until we merge this to ensure they have had enough time to comment.

@kirrg001 can you please take a look?

@kirrg001 did you have the chance to look at this PR?