-
Notifications
You must be signed in to change notification settings - Fork 540
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
|
18b5f3c
to
ea5f65e
Compare
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, 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.
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. |
ad119e1
to
839ae2e
Compare
839ae2e
to
41d533e
Compare
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 <[email protected]>
41d533e
to
bb657d4
Compare
Which problem is this PR solving?
Short description of the changes