Skip to content
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

Resource auto detection logging #1211

Merged

Conversation

adamegyed
Copy link
Contributor

Which problem is this PR solving?

Short description of the changes

  • Added ResourceDetectionConfig interface to be optionally taken in by the detectResources function. Currently only configurable field is the logger, which must adhere to the logging interface specified in opentelemetry-api

  • Added unit tests for logger functionality

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 17, 2020

CLA Check
The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #1211 into master will increase coverage by 0.30%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master    #1211      +/-   ##
==========================================
+ Coverage   92.71%   93.01%   +0.30%     
==========================================
  Files         129      131       +2     
  Lines        3471     3696     +225     
  Branches      712      747      +35     
==========================================
+ Hits         3218     3438     +220     
- Misses        253      258       +5     
Impacted Files Coverage Δ
...ry-resources/src/platform/node/detect-resources.ts 96.29% <94.44%> (-3.71%) ⬇️
...rces/src/platform/node/detectors/AwsEc2Detector.ts 87.50% <100.00%> (+0.40%) ⬆️
...sources/src/platform/node/detectors/EnvDetector.ts 95.55% <100.00%> (+7.46%) ⬆️
...sources/src/platform/node/detectors/GcpDetector.ts 95.34% <100.00%> (+0.22%) ⬆️
packages/opentelemetry-plugin-grpc/src/grpc.ts 96.79% <0.00%> (ø)
packages/opentelemetry-plugin-grpc/src/utils.ts 93.75% <0.00%> (ø)

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

I have one comment if we stick with the approach that is currently being taken.

An alternative approach that we might consider, is passing the ResourceDetectionConfig (with the logger) into each detector where they can make use it and add more detailed logging around events encountered internally. I'll let others weigh in on the merits of the different approaches.

@adamegyed adamegyed force-pushed the ResourceAutoDetectionLogging branch from e81575b to 4372c80 Compare June 19, 2020 00:15
@adamegyed
Copy link
Contributor Author

@mwear I've modified the Detector interface to optionally take in the ResourceDetectionConfig, but I'm not familiar with what information is valuable from each detector. Perhaps another contributor could make use of the logger within the detectors.

@adamegyed adamegyed requested a review from mwear June 23, 2020 22:23
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, they looks good with one thing only you don't need to create a new NoopLogger in each detector as the logger will already be on config then - I added comment to each of this case otherwise you will always have NoopLogger instead of user logger.
The last thing is adding a tsdoc for all exported items

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

In addition to the typo below, @obecny was asking for tsdoc updates. I think he's looking for documentation about the config parameter added to the detect methods.

Signed-off-by: Adam Egyed <[email protected]>
@adamegyed adamegyed requested a review from obecny June 26, 2020 21:03
Signed-off-by: Adam Egyed <[email protected]>
Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, thx for all the changes :)

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks @adamegyed!

@mayurkale22 mayurkale22 merged commit 4d358f2 into open-telemetry:master Jul 1, 2020
@dyladan dyladan added the enhancement New feature or request label Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add logging to Resource detection
6 participants