feat: resource auto-detection by mwear · Pull Request #899 · open-telemetry/opentelemetry-js

mwear · 2020-03-25T05:12:11Z

Which problem is this PR solving?

This an effort towards solving Resource auto-detection #841 (resource auto-detection)

Short description of the changes

The PR ports the existing resource auto-detection logic from OpenCensus. Some of the code has been reorganized and it uses the OpenTelemetry semantic conventions. For the most part, resource auto-detection is unspecified for OTel, so I've kept to the Census approach for the initial port.

The biggest change from the Census implementation is that I reorganized the detection logic for each Resource type into its own "detector" class. With the goal of making it easier to add detectors in the future.

For reference, here are links to the Census code this is based on:

Because there is potential latency involved in auto-detection, users need to manually call the detectResources method during setup, making this functionality opt-in. For example:

import { detectResources } from '@opentelemetry/resources'

const provider = new NodeTracerProvider({ resource: detectResources() });

codecov-io · 2020-03-25T05:18:47Z

Codecov Report

❌ Patch coverage is 96.39344% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.79%. Comparing base (8e56923) to head (689ffd3).

Files with missing lines	Patch %	Lines
...sources/src/platform/node/detectors/EnvDetector.ts	88.09%	5 Missing ⚠️
...rces/src/platform/node/detectors/AwsEc2Detector.ts	87.09%	4 Missing ⚠️
...sources/src/platform/node/detectors/GcpDetector.ts	95.12%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
+ Coverage   94.75%   94.79%   +0.04%     
==========================================
  Files         248      259      +11     
  Lines       11288    11590     +302     
  Branches     1079     1094      +15     
==========================================
+ Hits        10696    10987     +291     
- Misses        592      603      +11

Files with missing lines	Coverage Δ
packages/opentelemetry-resources/src/Resource.ts	`100.00% <100.00%> (ø)`
...ages/opentelemetry-resources/src/platform/index.ts	`100.00% <100.00%> (ø)`
...ry-resources/src/platform/node/detect-resources.ts	`100.00% <100.00%> (ø)`
...try-resources/src/platform/node/detectors/index.ts	`100.00% <100.00%> (ø)`
...opentelemetry-resources/src/platform/node/index.ts	`100.00% <100.00%> (ø)`
...ages/opentelemetry-resources/test/Resource.test.ts	`100.00% <100.00%> (ø)`
...ntelemetry-resources/test/detect-resources.test.ts	`100.00% <100.00%> (ø)`
...ry-resources/test/detectors/AwsEc2Detector.test.ts	`100.00% <100.00%> (ø)`
...metry-resources/test/detectors/EnvDetector.test.ts	`100.00% <100.00%> (ø)`
...metry-resources/test/detectors/GcpDetector.test.ts	`100.00% <100.00%> (ø)`
... and 4 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

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

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

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

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

mayurkale22

Overall LGTM, added a few comments in the first pass.

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

mayurkale22 · 2020-03-25T18:01:32Z

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

+ * EnvDetector can be used to detect the presence of and create a Resource
+ * from the OTEL_RESOURCE_LABELS environment variable.
+ */
+export class EnvDetector {


I think specs is not yet defined for the env detector. Perhaps, we can open an issue in specs to start the discussion.

Opened: open-telemetry/opentelemetry-specification#535

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

mayurkale22 · 2020-04-01T19:09:17Z

packages/opentelemetry-resources/package.json

    "@opentelemetry/api": "^0.5.2",
-    "@opentelemetry/base": "^0.5.2"
+    "@opentelemetry/base": "^0.5.2",
+    "gcp-metadata": "^3.5.0"


Minor: latest available version is 4.0.0.

It appears as though node 8 support was dropped in 4.0.0: https://github.com/googleapis/gcp-metadata/blob/master/CHANGELOG.md#-breaking-changes.

packages/opentelemetry-resources/src/types.ts

mayurkale22

Overall LGTM, thanks for the PR.

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

dyladan · 2020-04-01T19:42:39Z

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

+            try {
+              resolve(JSON.parse(rawData));
+            } catch (e) {
+              res.resume(); // consume response data to free up memory


There is no need to call resume after end event as far as I'm aware. Can you explain why you're calling it here?

This came from: https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-resource-util/src/resource-utils.ts#L153. I can remove it if it's useless.

From the node http docs:

if a 'response' event handler is added, then the data from the response object must be consumed, either by calling response.read() whenever there is a 'readable' event, or by adding a 'data' handler, or by calling the .resume() method. Until the data is consumed, the 'end' event will not fire.

To me, this implies that resume has nothing to consume when the 'end' event fires.

@mayurkale22 added that line to census so maybe he can weigh in?

I removed the res.resume() calls. I can add them back if we find they do have a benefit.

dyladan · 2020-04-01T19:46:33Z

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

+ */
+export const detectResources = async (): Promise<Resource> => {
+  const resources: Array<Resource> = await Promise.all(
+    DETECTORS.map(d => d.detect())


What happens if a detector throws?

I added a try / catch.

mayurkale22 · 2020-04-09T19:31:30Z

I think this is good to go, please resolve the merge conflicts.

vmarchaud

lgtm

obecny

lgtm, nice documentation!

…dCapacity is set to None (open-telemetry#899) * DynamoDB metrics for BatchGetItem fails when ReturnConsumedCapacity is set to NONE * fix(unit): add unit test and change if * fix(formatting): wrong formatting * fix(formatting): formatting Co-authored-by: Amir Blum <amir@aspecto.io>

mwear requested review from OlivierAlbertini, bg451, dyladan, markwolff, mayurkale22, obecny, rochdev and vmarchaud as code owners March 25, 2020 05:12

mwear commented Mar 25, 2020

View reviewed changes

packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts Outdated Show resolved Hide resolved

dyladan reviewed Mar 25, 2020

View reviewed changes

mayurkale22 reviewed Mar 25, 2020

View reviewed changes

mwear force-pushed the resource_autodetection branch 2 times, most recently from d0e0cf5 to db0df52 Compare April 1, 2020 19:12

mayurkale22 reviewed Apr 1, 2020

View reviewed changes

packages/opentelemetry-resources/src/types.ts Outdated Show resolved Hide resolved

mayurkale22 reviewed Apr 1, 2020

View reviewed changes

packages/opentelemetry-resources/src/types.ts Show resolved Hide resolved

mayurkale22 approved these changes Apr 1, 2020

View reviewed changes

dyladan reviewed Apr 1, 2020

View reviewed changes

mwear force-pushed the resource_autodetection branch 2 times, most recently from 1ebaab9 to a8486fc Compare April 4, 2020 03:29

dyladan approved these changes Apr 4, 2020

View reviewed changes

dyladan added the enhancement New feature or request label Apr 9, 2020

vmarchaud approved these changes Apr 10, 2020

View reviewed changes

obecny approved these changes Apr 10, 2020

View reviewed changes

dyladan added Has Merge Conflict labels Apr 11, 2020

mwear added 2 commits April 12, 2020 13:28

feat: add resource detection from environment variable

7d569da

feat: add AWS EC2 resource detection

8190a38

mwear added 18 commits April 12, 2020 13:29

feat: add resource detection for GCP

8c61b50

refactor: create Labels interface

2842f8e

feat: add Resource.detect() method

6325f1f

refactor: improve GcpDetector

d0007e7

feat: make detection platform aware; extract to detectResources() fn

9d54036

chore: cleanup

4ea66ec

chore: prefix private methods with _

2eb7aa2

fix: import URL for Node 8

c466a45

chore: prefix private constants with _

98a41fe

chore: do not export resources

e5d4ff1

chore: abort request on timeout

568d72a

refactor: export instances of detectors instead of static classes

d403261

refactor: formalize Detector interface

426ed85

refactor: rename Labels -> ResourceLabels

5d5b343

feat: gracefully handle detectors that throw

dce3718

fix: do not resume in an end event

b7f0592

docs: document interfaces and idenitity document url

7df23d5

fix: nock flakiness in resource tests

7a3ed94

mwear force-pushed the resource_autodetection branch from a8486fc to 7a3ed94 Compare April 12, 2020 20:30

Merge branch 'master' into resource_autodetection

689ffd3

dyladan removed Has Merge Conflict labels Apr 15, 2020

dyladan merged commit 182e9ee into open-telemetry:master Apr 15, 2020

mwear mentioned this pull request Apr 23, 2020

resource autodetection open-telemetry/opentelemetry-ruby#230

Closed

mayurkale22 mentioned this pull request May 13, 2020

Resource auto-detection #841

Closed

Conversation

mwear commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Short description of the changes

Uh oh!

codecov-io commented Mar 25, 2020 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mayurkale22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mayurkale22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayurkale22 commented Apr 9, 2020

Uh oh!

vmarchaud left a comment

Choose a reason for hiding this comment

Uh oh!

obecny left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mwear commented Mar 25, 2020 •

edited

Loading

codecov-io commented Mar 25, 2020 •

edited by codecov bot

Loading