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

fix Azure receiver not azure china #34402

Merged
merged 10 commits into from
Aug 12, 2024

Conversation

mo-silent
Copy link
Contributor

Description:

Add azure china to complete AzureMonitor Receiver

Link to tracking Issue:
#34315

mo-silent and others added 3 commits July 31, 2024 11:04
**Description:**

Adding azure china in azure monitor receiver.

**Link to tracking Issue:**

open-telemetry#34315
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Hello @mo-silent, can you please update the README as well to show this is a valid option for the cloud configuration option?

It would be good to add some kind of simple test for this as well.

.chloggen/mo-silent_fix-34315.yaml Outdated Show resolved Hide resolved
@mo-silent
Copy link
Contributor Author

Hi @crobert-1
Sure. I've updated README to add a valid option for cloud configuration options.

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Would you be able to add a simple test for this? (Sorry if you were still working on it, just commenting to make sure we didn't miss it.)

@mo-silent
Copy link
Contributor Author

mo-silent commented Aug 7, 2024

Hi @crobert-1
I'm not adding a new function. I just supplemented the Azure Cloud type by adding Azure China Cloud. So I don't think additional test code is needed, the existing test code uses defaultCloud, which is azureCloud. Again, there is no separate test case written for azureGovernmentCloud in the original test code.

@crobert-1
Copy link
Member

Hi @crobert-1 I'm not adding a new function. I just supplemented the Azure Cloud type by adding Azure China Cloud. So I don't think additional test code is needed, the existing test code uses defaultCloud, which is azureCloud. Again, there is no separate test case written for azureGovernmentCloud in the original test code.

I was thinking it would be good to have a test where the configuration is set to the new configuration option AzureChinaCloud, and then show that the *arm.ClientOptions object returned by getArmClientOptions is properly set to use the AzureChinaCloud. Let me know if that isn't possible, I had assumed it was a relatively simple test to add, but I might be missing something.

@mo-silent
Copy link
Contributor Author

Hi @crobert-1
I see what you mean. Have been added, called TestAzureScraperClientOptions test functions, and contains AzureChinaCloud, AzureCloud, AzureGovernmentCloud, the three cloud configuration options for the test cases.

@crobert-1
Copy link
Member

Thanks for adding the test @mo-silent!

From CI/CD:

azuremonitorreceiver/scraper_test.go:7: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
	"context"
azuremonitorreceiver/scraper_test.go:18: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
level=info msg="File cache stats: 17 entries of total size 95.8KiB"
level=info msg="Memory: 57 samples, avg is 198.3MB, max is 377.3MB"
level=info msg="Execution took 5.59690985s"
	"path/filepath"
	"reflect"
	"strings"
	"sync"
	"testing"
make[2]: *** [../../Makefile.Common:199: lint] Error 1
make[1]: *** [Makefile:179: receiver/azuremonitorreceiver] Error 2

This should be able to be resolved by running make gofmt

@mo-silent
Copy link
Contributor Author

Hi @crobert-1
Well. make gofmt has been executed.

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Thanks for resolving @mo-silent!

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @mo-silent!

@codeboten codeboten merged commit 4d04ddb into open-telemetry:main Aug 12, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 12, 2024
@@ -16,6 +16,7 @@ import (
const (
azureCloud = "AzureCloud"
azureGovernmentCloud = "AzureUSGovernment"
azureChinaCloud = "AzureChinaCloud"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add support for "AzureGermanCloud" as well, i'll open a follow up issue

f7o pushed a commit to f7o/opentelemetry-collector-contrib that referenced this pull request Sep 12, 2024
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.

4 participants