refactor: separate resource detector packages #1420

markwolff · 2020-08-13T02:10:20Z

Which problem is this PR solving?

Updates Move resource detectors into their own package #1411
- TODO: move to contrib in followup PR

Short description of the changes

Since there are a lot of diffs, I am keeping these packages in core repo until published(?), then they can safely be moved to contrib.
Adds optional detectors[] config to detect-resources.ts.
- node-sdk will fill these in with all of the "default" resource detectors.

markwolff · 2020-08-13T02:17:01Z

packages/opentelemetry-resource-detector-aws/package.json

@@ -0,0 +1,63 @@
+{
+  "name": "@opentelemetry/resource-detector-aws",


I split them based on cloud vendor, so ec2, beanstalk, etc would all live here.

packages/opentelemetry-sdk-node/test/sdk.test.ts

codecov · 2020-08-13T03:12:36Z

Codecov Report

Merging #1420 into master will decrease coverage by 0.77%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##           master    #1420      +/-   ##
==========================================
- Coverage   94.31%   93.53%   -0.78%     
==========================================
  Files         146      146              
  Lines        4396     4009     -387     
  Branches      893      774     -119     
==========================================
- Hits         4146     3750     -396     
- Misses        250      259       +9

Impacted Files	Coverage Δ
...resource-detector-gcp/src/detectors/GcpDetector.ts	`95.23% <ø> (ø)`
...ry-resources/src/platform/node/detect-resources.ts	`20.83% <0.00%> (-75.47%)`	⬇️
...sources/src/platform/node/detectors/EnvDetector.ts	`86.66% <33.33%> (-8.89%)`	⬇️
...ource-detector-aws/src/detectors/AwsEc2Detector.ts	`97.87% <100.00%> (ø)`
...metry-resource-detector-aws/src/detectors/index.ts	`100.00% <100.00%> (ø)`
...metry-resource-detector-gcp/src/detectors/index.ts	`100.00% <100.00%> (ø)`
packages/opentelemetry-sdk-node/src/sdk.ts	`83.05% <100.00%> (+8.05%)`	⬆️
packages/opentelemetry-plugin-http/src/http.ts
... and 4 more

…nto refactor/separate-resource-detectors

packages/opentelemetry-sdk-node/test/sdk.test.ts

markwolff · 2020-08-13T16:30:02Z

packages/opentelemetry-sdk-node/test/sdk.test.ts

@@ -128,4 +169,233 @@ describe('Node SDK', () => {
      assert.ok(metrics.getMeterProvider() instanceof MeterProvider);
    });
  });
+
+  describe('detectResources', async () => {


All of the old detect-resources.test.ts tests required the actual detectors, so I moved these tests from @opentelemetry/resources tests to sdk.test.ts. This move affects codecov, so eventually new tests with test-stub detectors should be added to the resources package

I agree. We should add a couple stub detectors like one that fails and one that succeeds.

markwolff · 2020-08-13T16:31:07Z

packages/opentelemetry-resources/src/platform/node/detect-resources.ts

-      const detectorName = DETECTORS[index].constructor
-        ? DETECTORS[index].constructor.name
-        : 'Unknown detector';
+      const detectorName = resource.detectedBy;


I've modified the Resource ctor to keep this functionality, but I don't think my change is ideal... /cc @mwear

The detectors have access to the logger (via the config) and log info during a failure. You could consider moving the logging for successful detections into the detector as well.

@mwear would that mean deleting this detectorName stuff altogether and letting the detectors handle the detector name stuff? i.e 81a07e2

Yeah, but I think you could go one step further, and have the Detector log the detected resources as they're found, rather than finding them all first and logging afterwards. Specifically, you could move this logging into the detector: https://github.com/open-telemetry/opentelemetry-js/pull/1420/files#diff-1328771aa7fff461aee162025e488e55R67-R79.

Do this only if you think it's an improvement though.

markwolff · 2020-08-13T16:32:27Z

packages/opentelemetry-sdk-node/src/sdk.ts

+  public async detectResources(config?: ResourceDetectionConfig) {
+    const internalConfig: ResourceDetectionConfig = {
+      ...config,
+      detectors: [awsEc2Detector, gcpDetector, envDetector],


now provide detectors via sdk config

dyladan · 2020-08-13T16:37:02Z

What do we think about keeping the env detector in the resources package? It is simple and has no external dependencies, and is required by the spec.

mwear

I had a couple of non-blocking questions / comments, but otherwise LGTM!

packages/opentelemetry-resource-detector-env/README.md

mwear · 2020-08-13T21:40:56Z

packages/opentelemetry-resources/src/platform/node/detect-resources.ts

-      const detectorName = DETECTORS[index].constructor
-        ? DETECTORS[index].constructor.name
-        : 'Unknown detector';
+      const detectorName = resource.detectedBy;


The detectors have access to the logger (via the config) and log info during a failure. You could consider moving the logging for successful detections into the detector as well.

packages/opentelemetry-resource-detector-aws/LICENSE

…nto refactor/separate-resource-detectors

packages/opentelemetry-resource-detector-aws/src/detectors/AwsEc2Detector.ts

dyladan · 2020-08-14T15:51:23Z

packages/opentelemetry-resource-detector-aws/src/detectors/AwsEc2Detector.ts

+          [HOST_RESOURCE.TYPE]: instanceType,
+        },
+      );
+      config.logger.debug(`${this.constructor.name} found resource.`);


Success and failure could be logged by the caller, which would make it consistent across detectors.

@dyladan Moved in b295d8d but required to remove try/catch from aws detector. ptal

dyladan · 2020-08-14T15:52:09Z

packages/opentelemetry-resource-detector-gcp/src/detectors/GcpDetector.ts

@@ -62,7 +63,10 @@ class GcpDetector implements Detector {
    if (process.env.KUBERNETES_SERVICE_HOST)
      this._addK8sLabels(labels, clusterName);

-    return new Resource(labels);
+    const resource = new Resource(labels);
+    config.logger.debug(`${this.constructor.name} found resource.`);


Same comment wrt logging by the caller

packages/opentelemetry-resources/src/platform/node/detect-resources.ts

dyladan · 2020-08-14T15:53:58Z

packages/opentelemetry-resources/src/platform/node/detectors/EnvDetector.ts

@@ -61,7 +64,9 @@ class EnvDetector implements Detector {
      const labels = this._parseResourceLabels(
        process.env.OTEL_RESOURCE_LABELS
      );
-      return new Resource(labels);
+      const resource = new Resource(labels);
+      config.logger.debug(`${this.constructor.name} found resource.`);


packages/opentelemetry-sdk-node/test/sdk.test.ts

dyladan · 2020-08-14T15:56:15Z

packages/opentelemetry-sdk-node/test/sdk.test.ts

@@ -128,4 +169,233 @@ describe('Node SDK', () => {
      assert.ok(metrics.getMeterProvider() instanceof MeterProvider);
    });
  });
+
+  describe('detectResources', async () => {


I agree. We should add a couple stub detectors like one that fails and one that succeeds.

…nto refactor/separate-resource-detectors

vmarchaud · 2020-08-15T14:40:59Z

packages/opentelemetry-resources/src/platform/node/detect-resources.ts

@@ -44,10 +40,13 @@ export const detectResources = async (
  );

  const resources: Array<Resource> = await Promise.all(
-    DETECTORS.map(d => {
+    (internalConfig.detectors || []).map(async d => {


nitpick but this || could be ??

vmarchaud

lgtm

…nto refactor/separate-resource-detectors

legendecas

I'm very fond of this PR as AWS/GCP detectors are slow in my environment most time. 👍

dyladan · 2020-08-18T12:03:51Z

Please resolve the conflicts and this can be merged

obecny · 2020-08-18T12:07:56Z

packages/opentelemetry-resource-detector-aws/README.md

+
+## Usage
+
+> TODO


dyladan · 2020-08-18T12:11:56Z

open-telemetry/opentelemetry-specification#811

…nto refactor/separate-resource-detectors

markwolff added 2 commits August 12, 2020 19:03

refactor: separate resource detector packages

6e99627

fix: add missing URL import

639b2ce

markwolff commented Aug 13, 2020

View reviewed changes

refactor: share duped test utils

e96ff75

markwolff marked this pull request as ready for review August 13, 2020 02:32

markwolff requested review from dyladan, legendecas, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners August 13, 2020 02:32

markwolff marked this pull request as draft August 13, 2020 02:56

legendecas reviewed Aug 13, 2020

View reviewed changes

packages/opentelemetry-sdk-node/test/sdk.test.ts Outdated Show resolved Hide resolved

markwolff added 2 commits August 12, 2020 23:00

fix: failing tests

637823b

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

1ccf14e

…nto refactor/separate-resource-detectors

markwolff commented Aug 13, 2020

View reviewed changes

packages/opentelemetry-sdk-node/test/sdk.test.ts Outdated Show resolved Hide resolved

markwolff commented Aug 13, 2020

View reviewed changes

chore: remove duped sinon

7dbec35

markwolff marked this pull request as ready for review August 13, 2020 16:35

fix: sinon typo

2e8251a

mwear approved these changes Aug 13, 2020

View reviewed changes

markwolff added 3 commits August 13, 2020 15:12

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

3b4c020

…nto refactor/separate-resource-detectors

refactor: migrate env detector

2816099

refactor: move detectedBy logging

81a07e2

dyladan reviewed Aug 14, 2020

View reviewed changes

markwolff added 5 commits August 14, 2020 09:08

style: fx misc type assertions

6a06ef8

refactor: mv detector logging to detect-resources

b295d8d

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

775eaa0

…nto refactor/separate-resource-detectors

fix: resolve merge conflict

47b5d1d

fix: failing tests

8d11fd2

vmarchaud reviewed Aug 15, 2020

View reviewed changes

vmarchaud approved these changes Aug 15, 2020

View reviewed changes

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

64b0e6d

…nto refactor/separate-resource-detectors

dyladan approved these changes Aug 17, 2020

View reviewed changes

legendecas reviewed Aug 18, 2020

View reviewed changes

legendecas approved these changes Aug 18, 2020

View reviewed changes

mayurkale22 approved these changes Aug 18, 2020

View reviewed changes

obecny reviewed Aug 18, 2020

View reviewed changes

packages/opentelemetry-resource-detector-aws/README.md

## Usage

> TODO

Copy link

Member

obecny Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

markwolff and others added 6 commits August 18, 2020 10:21

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

3dc5747

…nto refactor/separate-resource-detectors

fix: failing sdk test

229f6d5

Merge branch 'master' of github.com:open-telemetry/opentelemetry-js i…

2ea3ea5

…nto refactor/separate-resource-detectors

fix: aws detector tests

209c32b

fix: linting

650137d

Merge branch 'master' into refactor/separate-resource-detectors

12112fa

dyladan merged commit d68ff0f into open-telemetry:master Aug 19, 2020

markwolff deleted the refactor/separate-resource-detectors branch August 19, 2020 16:10

willarmiros mentioned this pull request Sep 13, 2021

chore: v1.0.0 proposal #2468

Merged

MSNev mentioned this pull request Mar 27, 2024

perf(otlp-transformer): reduce allocations #4578

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: separate resource detector packages #1420

refactor: separate resource detector packages #1420

markwolff commented Aug 13, 2020

markwolff Aug 13, 2020

codecov bot commented Aug 13, 2020 •

edited

Loading

markwolff Aug 13, 2020

dyladan Aug 14, 2020

markwolff Aug 13, 2020

mwear Aug 13, 2020

markwolff Aug 13, 2020 •

edited

Loading

mwear Aug 14, 2020

markwolff Aug 13, 2020

dyladan commented Aug 13, 2020

mwear left a comment

mwear Aug 13, 2020

dyladan Aug 14, 2020

markwolff Aug 14, 2020

dyladan Aug 14, 2020

dyladan Aug 14, 2020

dyladan Aug 14, 2020

vmarchaud Aug 15, 2020

vmarchaud left a comment

legendecas left a comment

dyladan commented Aug 18, 2020

obecny Aug 18, 2020

dyladan commented Aug 18, 2020

		@@ -0,0 +1,63 @@
		{
		"name": "@opentelemetry/resource-detector-aws",

refactor: separate resource detector packages #1420

refactor: separate resource detector packages #1420

Conversation

markwolff commented Aug 13, 2020

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

codecov bot commented Aug 13, 2020 • edited Loading

Codecov Report

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

markwolff Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Aug 13, 2020

mwear 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

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

vmarchaud left a comment

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

dyladan commented Aug 18, 2020

Choose a reason for hiding this comment

dyladan commented Aug 18, 2020

codecov bot commented Aug 13, 2020 •

edited

Loading

markwolff Aug 13, 2020 •

edited

Loading