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

feat(auto-instrumentation-node): add azure detector #2101

Merged
merged 3 commits into from Apr 29, 2024

Conversation

maryliag
Copy link
Contributor

Which problem is this PR solving?

  • The Azure detectors were not being added to the list of possible detectors. All other cloud providers were possible values, so this commit adds the missing detectors for Azure.

Short description of the changes

  • Adds dependency for azure detector and adds to the list of possible detector that can be added on the auto instrumentation node

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.45%. Comparing base (dfb2dff) to head (02ba5df).
Report is 87 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2101      +/-   ##
==========================================
- Coverage   90.97%   90.45%   -0.52%     
==========================================
  Files         146      147       +1     
  Lines        7492     7578      +86     
  Branches     1502     1574      +72     
==========================================
+ Hits         6816     6855      +39     
- Misses        676      723      +47     
Files Coverage Δ
...tapackages/auto-instrumentations-node/src/utils.ts 98.94% <100.00%> (+0.16%) ⬆️

... and 26 files with indirect coverage changes

Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@hectorhdzg hectorhdzg left a comment

Choose a reason for hiding this comment

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

LGTM, just I think this auto-instrumentation-node package is a little too much in my opinion, you don't expect customers to be running in Alibaba cloud, AWS and Azure at the same time, I know it can be configurable but having everything in same package could be overwhelming or confusing for some users.

@trentm
Copy link
Contributor

trentm commented Apr 17, 2024

you don't expect customers to be running in Alibaba cloud, AWS and Azure at the same time

I believe the intent is that the user doesn't need to think about configuring detectors. Whatever cloud env they run their app in, the relevant resource attrs will be detected.

I think right now the cloud detectors are noisy on diag.warn if they don't find anything. I'd like that to change to have them be silent if they don't find anything. However, that's a discussion for a separate issue.

@maryliag maryliag force-pushed the add-missing-detectors branch 2 times, most recently from ad119e1 to 839ae2e Compare April 23, 2024 12:33
The Azure detectors were not being added to the list of possible detectors. All other cloud providers
were possible values, so this commit adds the missing detectors for Azure.

Signed-off-by: maryliag <marylia.gutierrez@grafana.com>
@blumamir blumamir merged commit af2f3f1 into open-telemetry:main Apr 29, 2024
17 checks passed
@dyladan dyladan mentioned this pull request Apr 29, 2024
@maryliag maryliag deleted the add-missing-detectors branch April 29, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet