Support Tencent Cloud provider for add_cloud_metadata proccessor #4023

athom · 2017-04-16T23:30:25Z

Hey there,
Currently the add_cloud_metadata processor supports 3 main providers by default, it's awesome!

However, a lot of companies in China prefer to use Tencent Cloud and AliCloud. AFAIK, the two providers did hold a large marke share. It will be nice to support these two providers by default too.

This PR amis to support Tencent Cloud. Unlike the other 3 providers, Tencent Cloud Metadata behaves a bit different.
You can not get the full JSON format metadata in one single round trip, only one field of the metadata can by queried every request.
For example, you wanna to get the the metadata contains instance-id and region, you have to send two requests separately.

curl http://metadata.tencentyun.com/meta-data/instance-id

curl http://metadata.tencentyun.com/meta-data/placement/region

Here is the document

Summary of the changes:

refactor and abstract a data struct named fetcher who did the network/text processing job for specified provider.
generalize origin fetchJSON to fetcher.fetchRaw.
allow multiple requests to fetch the full metadata in the provider's fetcher level.
replace hard code of the goroutinue number in waiting loop with dynamic length cauculation.

Ps, I tested it on my qcloud instance with os ubuntu-16.04 , I can offer a instance for testing, feel free to ping me if you need one.

elasticmachine · 2017-04-16T23:32:02Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

elasticmachine · 2017-04-16T23:32:07Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

ruflin · 2017-04-18T07:02:53Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

@@ -31,10 +31,17 @@ const (

 	// Google GCE Metadata Service
 	gceMetadataURI = "/computeMetadata/v1/?recursive=true&alt=json"
+
+	// Tencent Clound Metadata Service
+	qcloudMetadataHost          = "metadata.tencentyun.com"


Now that we are starting to add more cloud providers it would be nice to have each provider in its own file. This makes it easier to spot which parts are specific to a provider and which is the shared code. Just a general note, could also be done in a follow up PR.

Good idea, I will apply it when implement AliCloud metadata in next PR ;)

ruflin

LGTM. I left some minor comments. Could you add an entry to CHANGELOG.asciidoc?

In case you do the refactoring with a future PR, I would appreciate if you open 2 PR's. One with the refactoring and one with the additional cloud provider so we can review it separately.

I would also like to get @andrewkroh review on this one.

@andrewkroh In case we add more and more providers, we should probably check again if all of them should be enabled by default.

ruflin · 2017-04-19T11:58:06Z

libbeat/docs/processors-config.asciidoc

@@ -266,11 +266,12 @@ The `add_cloud_metadata` processor enriches each event with instance metadata
 from the machine's hosting provider. At startup it will detect the hosting
 provider and cache the instance metadata.

-Three cloud providers are supported.
+Four cloud providers are supported.


We can probably drop the number here. More like The following cloud providers are supported:

Good suggestion

ruflin · 2017-04-19T11:59:30Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

+}
+
+func newCloudMetadata(c common.Config) (processors.Processor, error) {
+	timeout := 3 * time.Second


Could we introduce a constant for that so it is easy to find? Something like defaultTimeout?

This timeout value should be unpacked from the configuration data. Otherwise it is hardcoded here.

yep, it's hardcoded here, already move timeout configuration out of the getMetadataURLs and put it directly in newCloudMetadata.
The pool config is unpacked 5 times here 🤒, but I think it is acceptable here since it only run once in the startup phase.

andrewkroh · 2017-04-19T13:36:29Z

jenkins, test it

andrewkroh

@athom Thanks for the contribution! The refactoring looks good. It does need a change to read the timeout value from the config. My other comments are just minor doc issues and naming suggestions.

andrewkroh · 2017-04-19T13:39:07Z

libbeat/docs/processors-config.asciidoc


 - Amazon Elastic Compute Cloud (EC2)
 - Digital Ocean
 - Google Compute Engine (GCE)
+- [Tencent Cloud](https://www.qcloud.com/?lang=en) (QCloud)


I don't believe this is the proper format for a hyperlink in asciidoc. I think the format is http://google.com[Google Search].

🤦‍♂️ a clumsy product placement 😢
sorry for really green hand in asciidoc.

andrewkroh · 2017-04-19T13:43:56Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

+	qcloudMetadataHost          = "metadata.tencentyun.com"
+	qcloudMetadataInstanceIdURI = "/meta-data/instance-id"
+	qcloudMetadataRegionURI     = "/meta-data/placement/region"
+	qcloudMetadataZoneURI       = "/meta-data/placement/zone"


Do you know if they have versioned endpoints that we can point to (like /v1/meta-data/...) ? It would be preferable to use those if available so that there is less risk of the API changing.

AFAIK, there is no plan for Tentcent Qcloud endpoints to support /v1/xxx, the metadata service is maintained in my team :)
And the other provider AliCloud also has no versioned endpoints neither, at least no indications from the Chinese documents.

andrewkroh · 2017-04-19T13:50:21Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

-func (r result) String() string {
-	return fmt.Sprintf("result=[provider:%v, error=%v, metadata=%v]",
-		r.provider, r.err, r.metadata)
+type pick func([]byte, *result) error


Please add a godoc comment describing pick and its params and return value. And maybe change the name if you can think of something more descriptive (some ideas responseUnmarshaller, responseHandler).

good point, change applied.

andrewkroh · 2017-04-19T13:54:46Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

+	return
+}
+
+// fetchMetadata query metadata from a hosting provider's metadata service.


s/query/queries/

andrewkroh · 2017-04-19T13:55:53Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

+}
+
+// fetchMetadata query metadata from a hosting provider's metadata service.
+// some providers require multiple HTTP request to gather the whole metadata


s/some/Some/, s/request/requests/, and a period at the end of the line.

andrewkroh · 2017-04-19T14:19:41Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

-		MetadataHostAndPort: metadataHost,
-		Timeout:             3 * time.Second,
+		MetadataHostAndPort: defaultHost,
+		Timeout:             timeout,


This Timeout value is not used here so it needs to be relocated.

andrewkroh · 2017-04-19T14:21:07Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

+}
+
+func newCloudMetadata(c common.Config) (processors.Processor, error) {
+	timeout := 3 * time.Second


This timeout value should be unpacked from the configuration data. Otherwise it is hardcoded here.

andrewkroh · 2017-04-19T14:26:01Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

 )

 var debugf = logp.MakeDebug("filters")

+// metadata schemas for all prividers


s/prividers/providers./

andrewkroh · 2017-04-19T14:28:36Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

+			"region":            c.Str("region"),
+			"availability_zone": c.Str("zone"),
+		}.Apply(m)
+		return out


It seems like this doesn't even need to use the Schema as you have already built up the map[string]interface{} with the proper key names and data types. So I think this could just become return common.MapStr(m).

andrewkroh · 2017-04-19T14:29:56Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

 }

-// fetchJSON query metadata from a hosting provider's metadata service.
-func fetchJSON(
+// fetchRaw query raw metadata from a hosting provider's metadata service.


s/query/queries/

elasticmachine · 2017-04-19T14:49:56Z

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go

+
+	// Tencent Clound Metadata Service
+	qcloudMetadataHost          = "metadata.tencentyun.com"
+	qcloudMetadataInstanceIdURI = "/meta-data/instance-id"


[golint] _{reported by reviewdog 🐶}
const qcloudMetadataInstanceIdURI should be qcloudMetadataInstanceIDURI

andrewkroh · 2017-04-20T13:10:57Z

jenkins, test it

ruflin · 2017-04-21T06:52:53Z

@athom Thanks a lot for the contribution.

athom · 2017-04-22T09:11:56Z

Thanks for the merge

Support Tencent Cloud provider for add_cloud_metadata proccessor

d10a897

tsg added the review label Apr 17, 2017

ruflin reviewed Apr 18, 2017

View reviewed changes

monicasarbu approved these changes Apr 19, 2017

View reviewed changes

monicasarbu requested a review from andrewkroh April 19, 2017 10:44

ruflin reviewed Apr 19, 2017

View reviewed changes

andrewkroh requested changes Apr 19, 2017

View reviewed changes

elasticmachine reviewed Apr 19, 2017

View reviewed changes

update CHANGELOG and minor changes according to the review suggestions

a9bfde0

andrewkroh approved these changes Apr 20, 2017

View reviewed changes

ruflin merged commit be99062 into elastic:master Apr 21, 2017

athom deleted the support-qcloud-for-add-cloud-metadata branch April 22, 2017 09:12

athom mentioned this pull request Apr 22, 2017

File restructure by providers for add_cloud_metadata #4088

Merged

monicasarbu added :Processors libbeat labels Apr 24, 2017

athom mentioned this pull request Apr 25, 2017

Support Alibaba Cloud provider for add_cloud_metadata proccessor #4111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Tencent Cloud provider for add_cloud_metadata proccessor #4023

Support Tencent Cloud provider for add_cloud_metadata proccessor #4023

athom commented Apr 16, 2017 •

edited

Loading

elasticmachine commented Apr 16, 2017

elasticmachine commented Apr 16, 2017

ruflin Apr 18, 2017

athom Apr 18, 2017

ruflin left a comment

ruflin Apr 19, 2017

athom Apr 20, 2017

ruflin Apr 19, 2017

andrewkroh Apr 19, 2017

athom Apr 20, 2017 •

edited

Loading

andrewkroh commented Apr 19, 2017

andrewkroh left a comment

andrewkroh Apr 19, 2017

athom Apr 20, 2017

andrewkroh Apr 19, 2017

athom Apr 20, 2017

andrewkroh Apr 19, 2017

athom Apr 20, 2017

andrewkroh Apr 19, 2017

andrewkroh Apr 19, 2017

andrewkroh Apr 19, 2017

athom Apr 20, 2017

andrewkroh Apr 19, 2017

andrewkroh Apr 19, 2017

andrewkroh Apr 19, 2017

athom Apr 20, 2017

andrewkroh Apr 19, 2017

elasticmachine Apr 19, 2017

andrewkroh commented Apr 20, 2017

ruflin commented Apr 21, 2017

athom commented Apr 22, 2017

Support Tencent Cloud provider for add_cloud_metadata proccessor #4023

Support Tencent Cloud provider for add_cloud_metadata proccessor #4023

Conversation

athom commented Apr 16, 2017 • edited Loading

elasticmachine commented Apr 16, 2017

elasticmachine commented Apr 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athom Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

andrewkroh commented Apr 19, 2017

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Apr 20, 2017

ruflin commented Apr 21, 2017

athom commented Apr 22, 2017

athom commented Apr 16, 2017 •

edited

Loading

athom Apr 20, 2017 •

edited

Loading