[resourcedetectionprocessor] Azure detector #2372

mx-psi · 2021-02-18T14:56:26Z

Description:

Create Azure resource detector for gathering metadata from the Azure IMDS.

Due to Azure/azure-sdk-for-go#982 we have to query the endpoint manually.

Link to tracking Issue: Fixes #2326

Testing: Added unit tests. Tested end to end with provided sample configuration on an Azure VM.

Documentation: Documented detector on README

codecov · 2021-02-18T15:14:00Z

Codecov Report

Merging #2372 (e75ddd0) into main (944622e) will decrease coverage by 0.01%.
The diff coverage is 84.78%.

@@            Coverage Diff             @@
##             main    #2372      +/-   ##
==========================================
- Coverage   91.20%   91.18%   -0.02%     
==========================================
  Files         417      419       +2     
  Lines       20935    20981      +46     
==========================================
+ Hits        19093    19132      +39     
- Misses       1379     1382       +3     
- Partials      463      467       +4

Flag	Coverage Δ
integration	`69.22% <ø> (ø)`
unit	`90.06% <84.78%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ourcedetectionprocessor/internal/azure/metadata.go	`77.41% <77.41%> (ø)`
processor/resourcedetectionprocessor/factory.go	`98.70% <100.00%> (+0.01%)`	⬆️
...resourcedetectionprocessor/internal/azure/azure.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 944622e...e75ddd0. Read the comment docs.

mx-psi · 2021-02-18T14:59:41Z

processor/resourcedetectionprocessor/internal/azure/metadata.go

+}
+
+// computeMetadata is the Azure IMDS compute metadata response format
+type computeMetadata struct {


The struct does not contain every field returned by Azure, the rest of the fields are ignored since we use the default JSON decoder.

mx-psi · 2021-02-24T13:52:27Z

@anuraaga @jrcamp Could you take a look whenever you find the time? Thanks!

anuraaga

Sorry for the delay - lgtm

tigrannajaryan · 2021-02-24T14:13:53Z

@mx-psi can you take ownership of Azure detector? If yes please add yourself as codeowner for processor/resourcedetectionprocessor/internal/azure directory.

This reverts commit c51ef5f. It's easier to fix the merge conflict if I revert my commit first.

mx-psi · 2021-02-24T14:37:41Z

.github/CODEOWNERS

+processor/k8sprocessor/                              @open-telemetry/collector-contrib-approvers @owais @dmitryax @pmm-sumo
+processor/metricstransformprocessor/                 @open-telemetry/collector-contrib-approvers @james-bebbington
+processor/resourcedetectionprocessor/                @open-telemetry/collector-contrib-approvers @jrcamp @pmm-sumo @anuraaga @dashpole
+processor/resourcedetectionprocessor/internal/azure  @open-telemetry/collector-contrib-approvers @mx-psi


Added myself to CODEOWNERS here (I would recommend looking at the diff hiding whitespace changes)

mx-psi · 2021-02-24T14:40:07Z

@tigrannajaryan I am happy to do so, I added myself to the CODEOWNERS file :)

I am not sure if I will be pinged for reviews though since I don't have write access to the repository and that is a requirement according to Github docs. I will do my best to check PRs/issues manually in any case.

tigrannajaryan · 2021-02-24T20:50:44Z

@tigrannajaryan I am happy to do so, I added myself to the CODEOWNERS file :)

I am not sure if I will be pinged for reviews though since I don't have write access to the repository and that is a requirement according to Github docs. I will do my best to check PRs/issues manually in any case.

I am not sure either, but it is good to at least have you listed in the file so that we know who to ask questions. :-)

Thank you.

tigrannajaryan · 2021-02-24T20:51:00Z

@jrcamp do you plan to review or good to merge?

jrcamp

LGTM, just one question about the azure.* attributes.

jrcamp · 2021-02-24T23:55:52Z

processor/resourcedetectionprocessor/internal/azure/azure.go

+	attrs.InsertString(conventions.AttributeCloudRegion, compute.Location)
+	attrs.InsertString(conventions.AttributeHostID, compute.VMID)
+	attrs.InsertString(conventions.AttributeCloudAccount, compute.SubscriptionID)
+	attrs.InsertString("azure.vm.size", compute.VMSize)


@tigrannajaryan thoughts on these? Do we have any policy about using things that aren't in semantic conventions?

Yes, we do. See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/attribute-and-label-naming.md#recommendations-for-opentelemetry-authors and https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/attribute-and-label-naming.md#recommendations-for-application-developers

This seems applicable:

The name may be generally applicable to applications in the industry. In that case consider submitting a proposal to this specification to add a new name to the semantic conventions, and if necessary also to add a new namespace.

Or perhaps this:

The name is specific to your company and may be possibly used outside the company as well. To avoid clashes with names introduced by other companies (in a distributed system that uses applications from multiple vendors) it is recommended to prefix the new name by your company's reverse domain name, e.g. com.acme.shopname

Note that the Amazon ECS detector also uses attribute names that are not in the semantic conventions and they prefix them by aws.ecs.

I think the resource group one is fairly specific to how Azure works and has no equivalents on other cloud providers so it should have some Azure prefix (if azure is not okay, which one should be used?) . I thought about using host.type instead of azure.vm.size but it feels like a stretch.

You can keep it for now, but think about Azure attribute name conventions and if you can please make a proposal in spec repo.

tigrannajaryan · 2021-02-25T20:18:04Z

Please resolve merge conflicts.

mx-psi · 2021-02-25T21:48:25Z

@tigrannajaryan Done

mx-psi · 2021-03-03T18:31:47Z

@tigrannajaryan Could this be merged?

…#2372) Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.6.1 to 1.7.0. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.6.1...v1.7.0) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Create Azure resource detector for gathering metadata from the [Azure IMDS](https://aka.ms/azureimds). Due to Azure/azure-sdk-for-go#982 we have to query the endpoint manually. **Link to tracking Issue:** Fixes #2326 **Testing:** Added unit tests. Tested end to end with provided sample configuration on an Azure VM.

mx-psi added 3 commits February 18, 2021 15:38

Add metadata provider

fddddc1

Add Azure detector

c147817

Document new detector

70a349a

mx-psi commented Feb 18, 2021

View reviewed changes

mx-psi marked this pull request as ready for review February 18, 2021 15:15

mx-psi requested review from anuraaga and jrcamp as code owners February 18, 2021 15:15

mx-psi requested a review from a team February 18, 2021 15:15

github-actions bot assigned jpkrohling Feb 18, 2021

anuraaga approved these changes Feb 24, 2021

View reviewed changes

mx-psi added 4 commits February 24, 2021 15:30

Add myself as codeowner for the Azure resource detector

c51ef5f

Revert "Add myself as codeowner for the Azure resource detector"

7076340

This reverts commit c51ef5f. It's easier to fix the merge conflict if I revert my commit first.

Merge remote-tracking branch 'upstream/main' into mx-psi/azure-detector

ae5904e

Add myself as codeowner of the Azure detector

d98e478

mx-psi commented Feb 24, 2021

View reviewed changes

jrcamp reviewed Feb 24, 2021

View reviewed changes

Merge branch 'main' into mx-psi/azure-detector

40d1e26

Merge branch 'main' into mx-psi/azure-detector

1ab6a6e

mx-psi added 2 commits March 3, 2021 18:04

Merge branch 'main' into mx-psi/azure-detector

beda709

Retrigger CI

e75ddd0

tigrannajaryan merged commit fa60029 into open-telemetry:main Mar 3, 2021

mx-psi deleted the mx-psi/azure-detector branch March 4, 2021 07:54

mx-psi mentioned this pull request May 5, 2021

Track @mx-psi interest and contributions for approver open-telemetry/opentelemetry-collector#2819

Closed

swi-jared mentioned this pull request Jul 20, 2023

Add support for VM Instance Metadata Azure/azure-rest-api-specs#4408

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[resourcedetectionprocessor] Azure detector #2372

[resourcedetectionprocessor] Azure detector #2372

mx-psi commented Feb 18, 2021

codecov bot commented Feb 18, 2021 •

edited

Loading

mx-psi Feb 18, 2021

mx-psi commented Feb 24, 2021

anuraaga left a comment

tigrannajaryan commented Feb 24, 2021

mx-psi Feb 24, 2021 •

edited

Loading

mx-psi commented Feb 24, 2021

tigrannajaryan commented Feb 24, 2021

tigrannajaryan commented Feb 24, 2021

jrcamp left a comment

jrcamp Feb 24, 2021

tigrannajaryan Feb 25, 2021

mx-psi Feb 25, 2021

tigrannajaryan Feb 25, 2021

tigrannajaryan commented Feb 25, 2021

mx-psi commented Feb 25, 2021

mx-psi commented Mar 3, 2021

[resourcedetectionprocessor] Azure detector #2372

[resourcedetectionprocessor] Azure detector #2372

Conversation

mx-psi commented Feb 18, 2021

codecov bot commented Feb 18, 2021 • edited Loading

Codecov Report

mx-psi Feb 18, 2021

Choose a reason for hiding this comment

mx-psi commented Feb 24, 2021

anuraaga left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Feb 24, 2021

mx-psi Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

mx-psi commented Feb 24, 2021

tigrannajaryan commented Feb 24, 2021

tigrannajaryan commented Feb 24, 2021

jrcamp left a comment

Choose a reason for hiding this comment

jrcamp Feb 24, 2021

Choose a reason for hiding this comment

tigrannajaryan Feb 25, 2021

Choose a reason for hiding this comment

mx-psi Feb 25, 2021

Choose a reason for hiding this comment

tigrannajaryan Feb 25, 2021

Choose a reason for hiding this comment

tigrannajaryan commented Feb 25, 2021

mx-psi commented Feb 25, 2021

mx-psi commented Mar 3, 2021

codecov bot commented Feb 18, 2021 •

edited

Loading

mx-psi Feb 24, 2021 •

edited

Loading