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

Move add-cloud-metadata to ECS #9265

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Nov 28, 2018

Fields renamed:

  • meta.cloud.instance_id -> cloud.instance.id
  • meta.cloud.instance_name -> cloud.instance.name
  • meta.cloud.machine_type -> cloud.machine.type
  • meta.cloud.availability_zone -> cloud.availability_zone
  • meta.cloud.project_id -> cloud.project.id
  • meta.cloud.region -> cloud.region

Further changes:

  • Added alias for old fields
  • Update asset generation to include ECS assets in libbeat fields.yml. This is needed for testing as the aliases fields are in there.

@ruflin ruflin mentioned this pull request Nov 28, 2018
@ruflin ruflin force-pushed the add_cloud_metadata-to-ecs branch 4 times, most recently from 0b6df57 to 14d4495 Compare December 7, 2018 12:32
@ruflin ruflin added the Team:Integrations Label for the Integrations team label Dec 10, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/infrastructure

@ruflin ruflin self-assigned this Dec 10, 2018
@ruflin ruflin requested a review from webmat December 10, 2018 09:11
@ruflin ruflin force-pushed the add_cloud_metadata-to-ecs branch 3 times, most recently from 49e274d to 7dc4ef8 Compare December 12, 2018 15:29
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Almost there. The only small problem I see is that the meta.cloud.provider has been deleted instead of being made an alias to cloud.provider (both in migration file and actual common fields).

Once that alias is back in, I'm good with this.

Oh and I discovered another name clash with ECS ;-)

@@ -4,34 +4,32 @@
Metadata from cloud providers added by the add_cloud_metadata processor.
fields:

- name: meta.cloud.provider
Copy link
Contributor

Choose a reason for hiding this comment

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

meta.cloud.provider seems to be directly deleted, instead of becoming an alias to cloud.provider. Please add it back as an alias

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

- form: meta.cloud.region
to: cloud.region
alias: true
alias6: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you add the cloud.provider alias here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, not sure how that slipped through above :-(

"region": "cn-shenzhen",
"availability_zone": "cn-shenzhen-a",
"cloud": common.MapStr{
"provider": "ecs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh jeez. Another thing called ECS. After our schema and AWS' container service (yes, in that order). Moreover, Alibaba's thing is named "Elastic ..." as well.
💯 🤦‍♂️ 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, that one we knew ;-)

@webmat
Copy link
Contributor

webmat commented Dec 12, 2018

Restarted the single Travis test that was failing, and it passed. Likely a transient issue.

Not familiar enough with the provider tests to understand the failure in the Alibaba test:

10:44:45 ok  	github.com/elastic/beats/libbeat/outputs/transport/transptest	1.309s	coverage: 47.5% of statements
10:45:06 ok  	github.com/elastic/beats/libbeat/processors/actions	19.241s	coverage: 58.3% of statements
10:45:06 ok  	github.com/elastic/beats/libbeat/processors	19.828s	coverage: 68.3% of statements
10:45:06 command [go test -race -cover -coverprofile /tmp/gotestcover-323955738 github.com/elastic/beats/libbeat/processors/add_cloud_metadata]: exit status 1
10:45:06 --- FAIL: TestRetrieveAlibabaCloudMetadata (19.20s)
10:45:06     <autogenerated>:1: 
10:45:06         	Error Trace:	provider_alibaba_cloud_test.go:85
10:45:06         	Error:      	Not equal: 
10:45:06         	            	expected: common.MapStr{"cloud":common.MapStr{"provider":"ecs", "instance":common.MapStr{"id":"i-wz9g2hqiikg0aliyun2b"}, "region":"cn-shenzhen", "availability_zone":"cn-shenzhen-a"}}
10:45:06         	            	actual  : common.MapStr{}
10:45:06         	            	
10:45:06         	            	Diff:
10:45:06         	            	--- Expected
10:45:06         	            	+++ Actual
10:45:06         	            	@@ -1,2 +1,2 @@
10:45:06         	            	-(common.MapStr) (len=1) {"cloud":{"availability_zone":"cn-shenzhen-a","instance":{"id":"i-wz9g2hqiikg0aliyun2b"},"provider":"ecs","region":"cn-shenzhen"}}
10:45:06         	            	+(common.MapStr) {}
10:45:06         	            	 
10:45:06         	Test:       	TestRetrieveAlibabaCloudMetadata
10:45:06 FAIL
10:45:06 coverage: 85.2% of statements
10:45:06 FAIL	github.com/elastic/beats/libbeat/processors/add_cloud_metadata	19.331s
10:45:10 ok  	github.com/elastic/beats/libbeat/processors/add_host_metadata	1.025s	coverage: 83.1% of statements

Test run

@ruflin
Copy link
Member Author

ruflin commented Dec 12, 2018

@webmat Already tried to reproduce the above failing test locally but couldn't so far. Not sure how it's possible that it fails especially as it's based on local data. Will need to further investigate.

@webmat
Copy link
Contributor

webmat commented Dec 12, 2018

About the failing test: is there an actual API call being performed? Or is this based on a static fixture file of the API response?

May just be a network hiccup if there's an API call. I guess we'll see after this test run.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

If tests pass, this looks good to me

Fields renamed:

```
* meta.cloud.instance_id -> cloud.instance.id
* meta.cloud.instance_name -> cloud.instance.name
* meta.cloud.machine_type -> cloud.machine.type
* meta.cloud.availability_zone -> cloud.availability_zone
* meta.cloud.project_id -> cloud.project.id
* meta.cloud.region -> cloud.region
```

Further changes:

* Added alias for old fields
* Update asset generation to include ECS assets in libbeat fields.yml. This is needed for testing as the aliases fields are in there.
@ruflin ruflin merged commit 9354aa0 into elastic:master Dec 13, 2018
@ruflin ruflin deleted the add_cloud_metadata-to-ecs branch December 13, 2018 11:56
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
Fields renamed:

```
* meta.cloud.instance_id -> cloud.instance.id
* meta.cloud.instance_name -> cloud.instance.name
* meta.cloud.machine_type -> cloud.machine.type
* meta.cloud.availability_zone -> cloud.availability_zone
* meta.cloud.project_id -> cloud.project.id
* meta.cloud.region -> cloud.region
```

Further changes:

* Added alias for old fields
* Update asset generation to include ECS assets in libbeat fields.yml. This is needed for testing as the aliases fields are in there.
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.

3 participants