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.

Sign up for GitHub

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

Jump to bottom

[Metricbeat] Add storage metricset to Google Cloud Platform module #15598

Merged
merged 2 commits into from
Mar 24, 2020
Merged

[Metricbeat] Add storage metricset to Google Cloud Platform module #15598

merged 2 commits into from
Mar 24, 2020

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Jan 15, 2020 •
edited by andresrc
Loading

Add the storage metricset and the field region to the input config

cf. #15810

@sayden sayden requested a review from a team as a code owner January 15, 2020 20:26
@sayden sayden self-assigned this Jan 15, 2020
ChrsMark
ChrsMark reviewed Jan 16, 2020
View reviewed changes
x-pack/metricbeat/module/googlecloud/storage/_meta/data.json
},
"storage": {
"storage": {
"object_count": 1
Copy link
Member

@ChrsMark ChrsMark Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just a suggestion, since this is being included in the docs it would be nice if we could have more metrics-fields as a more complete sample for our users. I would try to include as many as possible of them https://github.com/elastic/beats/pull/15598/files#diff-71e43a13b3f822a4956f547a646aced4R10.

wdyt?

ChrsMark
ChrsMark reviewed Jan 16, 2020
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall lgtm @sayden already! I just left some minor concerns.

Also do you plan to add a dashboard too in this or in followup PR? It would be nice to update the PR's description with this info too.

x-pack/metricbeat/module/googlecloud/stackdriver/metadata_services.go Outdated
return nil, nil
case googlecloud.ServiceLoadBalancing:
return nil, nil
case googlecloud.ServiceStorage:
Copy link
Member

Choose a reason for hiding this comment

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

It seems that ServicePubsub and ServiceLoadBalancing are not strictly related to this patch and related to different upcoming metricsets, right?

Also ServiceStorage returns nil metadata too?

If we plan to keep it like this for now could you please provide some content here for reference so as to be more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, main thing here is that I'm trying to avoid too many potential conflicts because there are 3 different follow-up PR's touching the same function and I have to wait until only one is left to see overall final look.

About the question of storage: This file, metadata_services.go "controls / knows" which metricsets / gcp-services have an specific implementation to retrieve extra metadata (call it labels or properties) using their specific API's, instead of taking just the labels that are returned in the Stackdriver API. Only one metricset has such implementation: compute.

This is caused by a direct limitation on lightweight modules. Being so generic is a con in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough! Thanks for elaborating!

@andresrc andresrc requested a review from a team January 27, 2020 10:34
@kaiyan-sheng
Copy link
Contributor

Need a rebase and a changelog entry please 😬

@kaiyan-sheng kaiyan-sheng mentioned this pull request Feb 14, 2020
Merged
5 tasks
kaiyan-sheng
kaiyan-sheng reviewed Feb 19, 2020
View reviewed changes
x-pack/metricbeat/modules.d/googlecloud.yml.disabled
zone: "us-central1-a"
region: "us-central1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should compute and storage be in separate config section? Because one is taking zone and the other is taking region as config.

Copy link
Member

Choose a reason for hiding this comment

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

I would say that yes 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@sayden ^^ WDYT? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I overlooked at this the first time but yeap, I think it's better to separate them too. Thanks! 😬

@ChrsMark
Copy link
Member

@sayden @kaiyan-sheng could you please update the status of this after the changes/rebase?
It is floating around for some time now and I'm not sure if it is in_review status or there are still things to be done. So an update would be useful , if there is something blocking this or whatever 🙂 .

@sayden
Copy link
Contributor Author

sayden commented Feb 20, 2020 •
edited
Loading

It was blocked by the loadbalancing metricset and now it needs rebase. I'll do it asap It's still blocked after #16422

@sayden sayden force-pushed the feature/xp/mb/gcp/storage branch from a635414 to 9e87030 Compare March 4, 2020 18:56
@sayden
Copy link
Contributor Author

sayden commented Mar 16, 2020

@kaiyan-sheng whenever you can take a look at this, I'm not sure if the final "merge" is correct, for example the function constructFilter in metrics_requester.go is only being used in metric_requester_test.go. I'm not fully sure if this was intentional or I mess it up with conflicts so a second pair of eyes is more than welcome 😬

@kaiyan-sheng
Copy link
Contributor

@sayden Sorry for the late response. I believe constructFilter is covered in getFilterForMetric function now. You can remove constructFilter function and the unit test 😄

kaiyan-sheng
kaiyan-sheng approved these changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

We can create a separate PR to address the leftover comments 👍 Let's get this in first! Thanks!

@sayden sayden merged commit 52b9330 into elastic:master Mar 24, 2020
sayden added a commit to sayden/beats that referenced this pull request Mar 24, 2020
@sayden
[Metricbeat] Add storage metricset to Google Cloud Platform module (e…
3eedd4f 
…lastic#15598)

(cherry picked from commit 52b9330)

# Conflicts:
#	metricbeat/docs/modules/googlecloud.asciidoc
#	metricbeat/docs/modules_list.asciidoc
#	x-pack/metricbeat/metricbeat.reference.yml
#	x-pack/metricbeat/module/googlecloud/_meta/config.yml
#	x-pack/metricbeat/module/googlecloud/fields.go
#	x-pack/metricbeat/modules.d/googlecloud.yml.disabled
@sayden sayden mentioned this pull request Mar 24, 2020
Merged
@sayden sayden added the v7.7.0 label Mar 24, 2020
sayden added a commit that referenced this pull request Mar 24, 2020
@sayden
Cherry-pick #15598 to 7.x: [Metricbeat] Add storage metricset to Goog…
ebc46ce 
…le Cloud Platform module (#17219)
@sayden sayden added the test-plan Add this PR to be manual test plan label Mar 31, 2020
@andresrc andresrc added the test-plan-added This PR has been added to the test plan label Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrsMark ChrsMark ChrsMark left review comments

@kaiyan-sheng kaiyan-sheng kaiyan-sheng approved these changes

Assignees

@sayden sayden

Labels
enhancement Metricbeat Metricbeat Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan test-plan-added This PR has been added to the test plan v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sayden @kaiyan-sheng @ChrsMark @andresrc