-
Notifications
You must be signed in to change notification settings - Fork 753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: resource auto-detection #899
Conversation
Codecov Report
@@ 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
|
packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/node/detectors/EnvDetector.ts
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/node/detectors/GcpDetector.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, added a few comments in the first pass.
packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts
Show resolved
Hide resolved
* EnvDetector can be used to detect the presence of and create a Resource | ||
* from the OTEL_RESOURCE_LABELS environment variable. | ||
*/ | ||
export class EnvDetector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think specs is not yet defined for the env detector. Perhaps, we can open an issue in specs to start the discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/opentelemetry-resources/src/platform/node/detectors/GcpDetector.ts
Outdated
Show resolved
Hide resolved
d0e0cf5
to
db0df52
Compare
@@ -55,6 +60,7 @@ | |||
}, | |||
"dependencies": { | |||
"@opentelemetry/api": "^0.5.2", | |||
"@opentelemetry/base": "^0.5.2" | |||
"@opentelemetry/base": "^0.5.2", | |||
"gcp-metadata": "^3.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: latest available version is 4.0.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, thanks for the PR.
packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts
Show resolved
Hide resolved
packages/opentelemetry-resources/src/platform/node/detectors/AwsEc2Detector.ts
Show resolved
Hide resolved
try { | ||
resolve(JSON.parse(rawData)); | ||
} catch (e) { | ||
res.resume(); // consume response data to free up memory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the res.resume()
calls. I can add them back if we find they do have a benefit.
*/ | ||
export const detectResources = async (): Promise<Resource> => { | ||
const resources: Array<Resource> = await Promise.all( | ||
DETECTORS.map(d => d.detect()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a detector throws?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a try / catch.
1ebaab9
to
a8486fc
Compare
I think this is good to go, please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, nice documentation!
a8486fc
to
7a3ed94
Compare
…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>
Which problem is this PR solving?
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: