feat(detector-alibaba)!: change implementation to DetectorSync interface #2328

david-luna · 2024-07-12T08:08:02Z

Context

In order to fix #2320 we need to suppress tracing when this detector is doing HTTP requests or using any other instrumented API. IMO this would be easier if we already have the detectors implementing the DetectorSync interface moving away from the deprecated Detector. This PR is adding a new sync detector that will be used by the SDK and auto-instrumentations package (to be changed in a follow-up PR).

Follow-up PRs will be created to:

suppressTracing
use the sync detector in auto-instrumentations

Short description of the changes

Make the current detector implement the DetectorSync interface
update tests

Test changes

The new detector's detect method becomes synchronous and therefore there is no way to throw an exception in it for an async HTTP error. The refactor makes this detector to return and empty resource if an async error occurs, so the test does not expect an exception anymore but an empty resource.

Also this aligns with @opentelemetry/resources logic used by the NodeSDK which catches any exception from the detector and returns an empty resource.
https://github.com/open-telemetry/opentelemetry-js/blob/54b14fbbe33e0cf38115ee8c5c934a4e27786186/packages/opentelemetry-resources/src/detect-resources.ts#L32

…erface

codecov · 2024-07-12T08:19:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (dfb2dff) to head (fe26750).
Report is 199 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2328      +/-   ##
==========================================
- Coverage   90.97%   90.40%   -0.57%     
==========================================
  Files         146      149       +3     
  Lines        7492     7361     -131     
  Branches     1502     1527      +25     
==========================================
- Hits         6816     6655     -161     
- Misses        676      706      +30

Files	Coverage Δ
...aba-cloud/src/detectors/AlibabaCloudEcsDetector.ts	`97.77% <100.00%> (+0.10%)`	⬆️
...urce-detector-alibaba-cloud/src/detectors/index.ts	`100.00% <100.00%> (ø)`

... and 66 files with indirect coverage changes

legendecas

The @opentelemetry/resources version requirement in the package.json needs to be updated as ^1.10.0.

...pentelemetry-resource-detector-alibaba-cloud/test/fixtures/use-alibaba-cloud-ecs-detector.js

legendecas · 2024-07-12T10:34:31Z

.../node/opentelemetry-resource-detector-alibaba-cloud/src/detectors/AlibabaCloudEcsDetector.ts

+        [SEMRESATTRS_HOST_TYPE]: instanceType,
+        [SEMRESATTRS_HOST_NAME]: hostname,
+      };
+    } catch {


Could a diag log be added for the error?

Given that the rejection is handled at https://github.com/open-telemetry/opentelemetry-js/blob/54b14fbbe33e0cf38115ee8c5c934a4e27786186/packages/opentelemetry-resources/src/Resource.ts#L87-L91. I think we should propagate the error instead of discarding it here.

legendecas · 2024-07-12T10:41:35Z

I think a more non-breaking way would be add a new AlibabaCloudEcsDetectorSync given that https://github.com/open-telemetry/opentelemetry-js/blob/54b14fbbe33e0cf38115ee8c5c934a4e27786186/packages/opentelemetry-resources/src/types.ts#L31-L41 the DetectorSync has a different method signature (return type) of detect.

…est/fixtures/use-alibaba-cloud-ecs-detector.js Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

david-luna · 2024-07-29T17:03:51Z

I think a more non-breaking way would be add a new AlibabaCloudEcsDetectorSync given that https://github.com/open-telemetry/opentelemetry-js/blob/54b14fbbe33e0cf38115ee8c5c934a4e27786186/packages/opentelemetry-resources/src/types.ts#L31-L41 the DetectorSync has a different method signature (return type) of detect.

@legendecas thanks for the suggestion. I'll apply the changes here and bring it up to the JS SIG to get more feedback and, if accepted, apply it also to the other detectors.

trentm · 2024-07-30T00:37:20Z

THe diff got big because the implementation moved from the ...Detector.ts to ...DetectorSync.ts file. If it helps, here is the diff between the current Detector.ts file on "main" and the DetectorSync.ts file in this PR:

the diff

--- /Users/trentm/src/opentelemetry-js-contrib/detectors/node/opentelemetry-resource-detector-alibaba-cloud/src/detectors/AlibabaCloudEcsDetector.ts	2024-04-16 11:49:08
+++ AlibabaCloudEcsDetectorSync.ts	2024-07-29 17:34:07
@@ -15,8 +15,10 @@
  */

 import {
-  Detector,
+  DetectorSync,
+  IResource,
   Resource,
+  ResourceAttributes,
   ResourceDetectionConfig,
 } from '@opentelemetry/resources';
 import {
@@ -38,7 +40,7 @@
  * AlibabaCloud ECS and return a {@link Resource} populated with metadata about
  * the ECS instance. Returns an empty Resource if detection fails.
  */
-class AlibabaCloudEcsDetector implements Detector {
+class AlibabaCloudEcsDetectorSync implements DetectorSync {
   /**
    * See https://www.alibabacloud.com/help/doc-detail/67254.htm for
    * documentation about the AlibabaCloud instance identity document.
@@ -57,7 +59,14 @@
    *
    * @param config (unused) The resource detection config
    */
-  async detect(_config?: ResourceDetectionConfig): Promise<Resource> {
+  detect(_config?: ResourceDetectionConfig): IResource {
+    return new Resource({}, this._getAttributes());
+  }
+
+  /** Gets identity and host info and returns them as attribs. Empty object if fails */
+  async _getAttributes(
+    _config?: ResourceDetectionConfig
+  ): Promise<ResourceAttributes> {
     const {
       'owner-account-id': accountId,
       'instance-id': instanceId,
@@ -67,7 +76,7 @@
     } = await this._fetchIdentity();
     const hostname = await this._fetchHost();

-    return new Resource({
+    return {
       [SEMRESATTRS_CLOUD_PROVIDER]: CLOUDPROVIDERVALUES_ALIBABA_CLOUD,
       [SEMRESATTRS_CLOUD_PLATFORM]: CLOUDPLATFORMVALUES_ALIBABA_CLOUD_ECS,
       [SEMRESATTRS_CLOUD_ACCOUNT_ID]: accountId,
@@ -76,7 +85,7 @@
       [SEMRESATTRS_HOST_ID]: instanceId,
       [SEMRESATTRS_HOST_TYPE]: instanceType,
       [SEMRESATTRS_HOST_NAME]: hostname,
-    });
+    };
   }

   /**
@@ -144,4 +153,4 @@
   }
 }

-export const alibabaCloudEcsDetector = new AlibabaCloudEcsDetector();
+export const alibabaCloudEcsDetectorSync = new AlibabaCloudEcsDetectorSync();

trentm · 2024-07-30T00:54:14Z

...de/opentelemetry-resource-detector-alibaba-cloud/test/detectors/AlibabaCloudDetector.test.ts

Should this test file (of the old, unchanged non-Sync detector) be reverted back to its original state?

(I wonder if some of the tests below are passing because of luck.)

@trentm

Since the async detector is leveraging the new sync detector there's a change in behavior that makes the original tests fail. The difference is the detector's detect method does not throw anymore.

I guess that's something we do not want. I'll share it in the upcoming SIG to get feedback.

david-luna · 2024-07-31T18:02:41Z

Hi @legendecas

in today's JavaScript SIG we discussed this and the other detector PRs. The conclusion was it's okay to do a breaking change for non stable detectors if the owner is okay with it. In this detector you suggested a non breaking change so I've added another detector complying with DetectorSync interface and export both.

It's worth mentioning the former detector reused the new one following the pattern existing in the core repo (example). With this approach the detector does not throw but returns an empty resource, tests are changed accordingly. This was also discussed in the SIG and agreed that the change of behavior should not be considered a breaking change.

Also since were adding a new detector I'll rename this PR to feat instead of refactor

I think trent's comment points to what is the main change here.

legendecas · 2024-07-31T19:41:25Z

Thanks for the update! I think I am fine with breaking if we do all the same with other detectors, given that the test might be more complex if we have two variants.

trentm

Just the test file name nit.

trentm · 2024-07-31T21:51:59Z

...de/opentelemetry-resource-detector-alibaba-cloud/test/detectors/AlibabaCloudDetector.test.ts

The "Ecs" in the filename was accidentally removed in the shuffling?

yup, my bad

trentm · 2024-07-31T21:58:34Z

I approved, but I guess we are now considering dropping the ...Detector non-Sync classes for all the non-stable detectors. I'm cool with that as well.

david-luna · 2024-08-01T09:44:27Z

Thanks @legendecas & @trentm for your feedback. I think dropping the async detector is the best course of action now that we agree on it.

will make the suppressTracing easier in the follow up PR
will simplify work on Consolidate sync and async resource interfaces opentelemetry-js#3582 which is part of the SDK 2.0 milestone

So I'm going to revert the changes to the 1st proposal and refactor the detector from async to sync.

david-luna · 2024-08-01T09:59:02Z

detectors/node/opentelemetry-resource-detector-alibaba-cloud/src/index.ts

@@ -14,4 +14,4 @@
 * limitations under the License.
 */

-export * from './detectors';
+export { alibabaCloudEcsDetector } from './detectors';


note to reviewer: little step forward to resolve open-telemetry/opentelemetry-js#4186

david-luna · 2024-08-01T10:00:34Z

...opentelemetry-resource-detector-alibaba-cloud/test/detectors/AlibabaCloudEcsDetector.test.ts

@@ -84,56 +85,47 @@ describe('alibabaCloudEcsDetector', () => {
  });

  describe('with unsuccessful request', () => {
-    it('should throw when receiving error response code', async () => {
-      const expectedError = new Error('Failed to load page, status code: 404');
+    it('should return empty resource when receiving error response code', async () => {


note to reviewer: the detector does not throw but rather returns a resource with no attribs.

refactor(detector-alibaba): change implementation to DetectorSync int…

3389dcc

…erface

david-luna requested a review from a team July 12, 2024 08:08

github-actions bot added the pkg:resource-detector-alibaba-cloud label Jul 12, 2024

github-actions bot assigned legendecas Jul 12, 2024

github-actions bot requested a review from legendecas July 12, 2024 08:10

This was referenced Jul 12, 2024

feat(detector-aws): add DetectorSync implementation for Lambda detector #2329

Closed

feat(detector-aws): add DetectorSync implementation for beanstalk detector #2330

Closed

feat(detector-aws): add DetectorSync implementation for ECS detector #2331

Closed

legendecas reviewed Jul 12, 2024

View reviewed changes

This was referenced Jul 12, 2024

feat(detector-aws): add DetectorSync implementation for EC2 detector #2332

Closed

feat(detector-aws): add DetectorSync implementation for EKS detector #2333

Closed

refactor(detector-alibaba): fix return type

0fd9ba3

This was referenced Jul 12, 2024

feat(detector-container)!: change implementation to DetectorSync interface #2334

Merged

feat(detector-gcp)!: change implementation to DetectorSync interface #2335

Merged

feat(detector-github)!: change implementation to DetectorSync interface #2336

Merged

david-luna and others added 4 commits July 15, 2024 09:49

Update detectors/node/opentelemetry-resource-detector-alibaba-cloud/t…

3ded77a

…est/fixtures/use-alibaba-cloud-ecs-detector.js Co-authored-by: Chengzhong Wu <legendecas@gmail.com>

chore(detector-alibaba): remove unused fixture file

2b50ea1

chore(detector-alibaba): sync changes

72663b9

chore(detector-alibaba): update dependencies

873b9a5

github-actions bot assigned basti1302 and kirrg001 Jul 15, 2024

github-actions bot requested review from basti1302 and kirrg001 July 15, 2024 08:25

github-actions bot added the pkg:resource-detector-instana label Jul 15, 2024

david-luna added 2 commits July 29, 2024 19:03

Merge branch 'main' into dluna/aibaba-ecs-detector-sync

e672c53

refactor(detector-alibaba): avoid breaking change in API

42862a8

trentm reviewed Jul 30, 2024

View reviewed changes

david-luna added 2 commits July 30, 2024 18:30

chore(detector-alibaba): fix lint issue

baaa6ee

chore(detector-alibaba): move constants to a separate file

25bcb8d

david-luna requested review from legendecas and trentm July 31, 2024 18:04

david-luna changed the title ~~refactor(detector-alibaba): change implementation to DetectorSync interface~~ feat(detector-alibaba): add AlibabaCloudDetectorSync Jul 31, 2024

trentm approved these changes Jul 31, 2024

View reviewed changes

chore(detector-alibaba): remove async detector

fdd6524

david-luna commented Aug 1, 2024

View reviewed changes

chore(detector-alibaba): reset test name

57bf61a

david-luna commented Aug 1, 2024

View reviewed changes

david-luna changed the title ~~feat(detector-alibaba): add AlibabaCloudDetectorSync~~ feat(detector-alibaba)!: change implementation to DetectorSync interface Aug 1, 2024

david-luna added 2 commits August 1, 2024 12:11

Merge branch 'main' into dluna/aibaba-ecs-detector-sync

ac92a5c

Merge branch 'main' into dluna/aibaba-ecs-detector-sync

fe26750

david-luna merged commit 25e85c7 into open-telemetry:main Aug 2, 2024
21 checks passed

david-luna deleted the dluna/aibaba-ecs-detector-sync branch August 2, 2024 07:25

dyladan mentioned this pull request Aug 2, 2024

chore: release main #2362

Merged

david-luna mentioned this pull request Aug 5, 2024

traces of resource detectors being sent #2320

Open

david-luna mentioned this pull request Sep 12, 2024

fix(detector-alibaba): suppres internal tracing #2429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(detector-alibaba)!: change implementation to DetectorSync interface #2328

feat(detector-alibaba)!: change implementation to DetectorSync interface #2328

david-luna commented Jul 12, 2024 •

edited

Loading

codecov bot commented Jul 12, 2024 •

edited

Loading

legendecas left a comment

legendecas Jul 12, 2024

legendecas Jul 12, 2024

david-luna Jul 15, 2024

legendecas commented Jul 12, 2024

david-luna commented Jul 29, 2024

trentm commented Jul 30, 2024

trentm Jul 30, 2024

david-luna Jul 30, 2024

david-luna commented Jul 31, 2024 •

edited

Loading

legendecas commented Jul 31, 2024

trentm left a comment

trentm Jul 31, 2024

david-luna Aug 1, 2024

trentm commented Jul 31, 2024

david-luna commented Aug 1, 2024 •

edited

Loading

david-luna Aug 1, 2024

david-luna Aug 1, 2024

feat(detector-alibaba)!: change implementation to DetectorSync interface #2328

feat(detector-alibaba)!: change implementation to DetectorSync interface #2328

Conversation

david-luna commented Jul 12, 2024 • edited Loading

Context

Short description of the changes

Test changes

codecov bot commented Jul 12, 2024 • edited Loading

Codecov Report

legendecas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legendecas commented Jul 12, 2024

david-luna commented Jul 29, 2024

trentm commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-luna commented Jul 31, 2024 • edited Loading

legendecas commented Jul 31, 2024

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentm commented Jul 31, 2024

david-luna commented Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-luna commented Jul 12, 2024 •

edited

Loading

codecov bot commented Jul 12, 2024 •

edited

Loading

david-luna commented Jul 31, 2024 •

edited

Loading

david-luna commented Aug 1, 2024 •

edited

Loading