x-pack/metricbeat/module/gcp: Add Organization ID and display name to cloud labels #40461

Linu-Elias · 2024-08-08T11:28:46Z

Proposed commit message

Set the project ID in the cloud.project.id field (currently missing)
Set the Google Cloud ORG ID in the cloud.account.id field (currently contains the project ID)
Set the Google Cloud ORG display name in the cloud.account.name field (currently contains the project ID)

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

[ ]

How to test this PR locally

Related issues

Closes GCP metricset does not collect Google Cloud Org ID and display name #39203

Use cases

Screenshots

With Project that belong to an Organization:

With Project that belong to "No Organization Category" and Compute Service
With Project that belong to "No Organization Category" and Spanner Service

Logs

botelastic · 2024-08-08T11:28:52Z

This pull request doesn't have a Team:<team> label.

mergify · 2024-08-08T11:29:21Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @Linu-Elias? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

x-pack/metricbeat/module/gcp/metrics/metricset.go

CHANGELOG.next.asciidoc

mergify · 2024-08-13T00:34:44Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b gcp-cloud-label-fix upstream/gcp-cloud-label-fix
git merge upstream/main
git push upstream gcp-cloud-label-fix

x-pack/metricbeat/module/gcp/timeseries_metadata_collector.go

mergify · 2024-08-23T06:58:58Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b gcp-cloud-label-fix upstream/gcp-cloud-label-fix
git merge upstream/main
git push upstream gcp-cloud-label-fix

kaiyan-sheng · 2024-08-26T16:56:12Z

CHANGELOG.next.asciidoc

@@ -176,6 +176,7 @@ https://github.com/elastic/beats/compare/v8.8.1\...main[Check the HEAD diff]
 - Update beat module with apm-server monitoring metrics fields {pull}40127[40127]
 - Fix Azure Monitor metric timespan to restore Storage Account PT1H metrics {issue}40376[40376] {pull}40367[40367]
 - Remove excessive info-level logs in cgroups setup {pull}40491[40491]
+- Add GCP organization ID and display name to ECS cloud fields. {issue}39203[39203] {pull}40461[40461]


Should this be a bug fix or enhancement?

Yeah, it's a challenging call. It feels both.

enhancement: we're adding project IDs and names that were unavailable.

bugfix: setting organization IDs and names with the correct values, according to ECS. This feels like an alignment with the spec and part of a fix.

IMO, this change is 75% an enhancement and 25% a fix. It's not a blocker; I'll leave the final call to you, @Linu-Elias.

kaiyan-sheng

Overall it looks good. Can we add some documentation on the new fields and also change the existing field definition to reflect this change please? Thank you!

Linu-Elias · 2024-08-27T05:32:11Z

Overall it looks good. Can we add some documentation on the new fields and also change the existing field definition to reflect this change please? Thank you!

@kaiyan-sheng I'm not sure if you’re referring to this Documentation, but we already have appropriate field definitions here.

zmoog

The PR looks good; thank you for working on this, @Linu-Elias!

Besides the comments about the constant name and the additional API call to get the project name, we should pay attention to one aspect: the rollout to users.

This change is a breaking change (by design).

The cloud.project.id field is OK; we're adding a missing field.

However, the cloud.account.id and cloud.account.name fields now contain the correct values instead of the project ID. Even if wrong, users may have grown using them.

@lucabelluccini, I know it is not your responsibility, but you probably have experience with problematic rollouts, so you are my expert.

What's the best way to communicate a change in field availability and value?

In this PR, we are:

adding a new field cloud.project.id (and maybe cloud.project.name)
fixing the cloud.account.id and cloud.account.name fields, setting them with the actual organization ID and name instead of the project ID
cloud.account.id and cloud.account.name may not be available (the service account may lack the required permissions to get the information), while before this change, they were always present.

x-pack/metricbeat/module/gcp/metrics/metricset.go

zmoog · 2024-08-30T14:09:17Z

x-pack/metricbeat/module/gcp/timeseries_metadata_collector.go

@@ -53,14 +57,19 @@ func (s *StackdriverTimeSeriesMetadataCollector) Metadata(ctx context.Context, i

 	ecs := mapstr.M{
 		ECSCloud: mapstr.M{
-			ECSCloudAccount: mapstr.M{
+			ECSCloudProject: mapstr.M{
 				ECSCloudAccountID:   accountID,
 				ECSCloudAccountName: accountID,


I think we need to change the names of these constants; even if the values are correct ("id" and "name"), the name is misleading.

They contain "CloudAccount" while we use them for the ECSCloudProject:

ECSCloudAccountID = "id" ECSCloudAccountName = "name"

The ECSCloudAccountID and ECSCloudAccountName is used in two places - for setting organization(id and name) and Project(id and name) hence the field name can be something generic like- ECSCloudID and ECSCloudName ?

x-pack/metricbeat/module/gcp/timeseries_metadata_collector.go

lucabelluccini · 2024-08-30T15:40:20Z

What's the best way to communicate a change in field availability and value?

In this PR, we are:

adding a new field cloud.project.id (and maybe cloud.project.name)

fixing the cloud.account.id and cloud.account.name fields, setting them with the actual organization ID and name instead of the project ID

cloud.account.id and cloud.account.name may not be available (the service account may lack the required permissions to get the information), while before this change, they were always present.

Thanks @zmoog on the callout!

I think:

Make this clear in the changelog/release notes of both GCP Module & GCP Integration as much as possible
This is a good enhancement/fix, but I see eventually some users who might complain of the change. As before all fields were identical, I think we might leverage one of the following:
- A custom ingest pipeline to put the fields as they were (just processors to copy the cloud.project.id into cloud.account.id and cloud.account.name)
- Runtime fields to shadow the real content of the fields with the "old" value (just define 2 runtime fields cloud.account.id and cloud.account.name, making them read the value from cloud.project.id)

It would be good to have those 2 workarounds tested (if needed I can help setting them up if you have an accessible cluster where you have the data) and documented in a KB or in the docs.

zmoog

LGTM!

We need to add two follow-up actions to the #39203 issue to implement the suggestions from @lucabelluccini.

I think:

Make this clear in the changelog/release notes of both GCP Module & GCP Integration as much as possible

This is a good enhancement/fix, but I see eventually some users who might complain of the change. As before all fields were identical, I think we might leverage one of the following:

A custom ingest pipeline to put the fields as they were (just processors to copy the cloud.project.id into cloud.account.id and cloud.account.name)

Runtime fields to shadow the real content of the fields with the "old" value (just define 2 runtime fields cloud.account.id and cloud.account.name, making them read the value from cloud.project.id)

It would be good to have those 2 workarounds tested (if needed I can help setting them up if you have an accessible cluster where you have the data) and documented in a KB or in the docs.

We need to update the release notes for the GCP module
We need to update the GCP integration docs
Provide a tested copy-and-paste solution to revert this change for those users who may rely on the previous behavior (I lean towards the custom pipeline, mainly because it feels easier to understand, but maybe I'm biased).

@Linu-Elias, we can work on these follow-up actions separately from this PR, which is ready.

zmoog · 2024-09-09T20:52:21Z

CHANGELOG.next.asciidoc

@@ -176,6 +176,7 @@ https://github.com/elastic/beats/compare/v8.8.1\...main[Check the HEAD diff]
 - Update beat module with apm-server monitoring metrics fields {pull}40127[40127]
 - Fix Azure Monitor metric timespan to restore Storage Account PT1H metrics {issue}40376[40376] {pull}40367[40367]
 - Remove excessive info-level logs in cgroups setup {pull}40491[40491]
+- Add GCP organization ID and display name to ECS cloud fields. {issue}39203[39203] {pull}40461[40461]


Yeah, it's a challenging call. It feels both.

enhancement: we're adding project IDs and names that were unavailable.

bugfix: setting organization IDs and names with the correct values, according to ECS. This feels like an alignment with the spec and part of a fix.

IMO, this change is 75% an enhancement and 25% a fix. It's not a blocker; I'll leave the final call to you, @Linu-Elias.

x-pack/metricbeat/module/gcp/metrics/metricset.go

x-pack/metricbeat/module/gcp/_meta/docs.asciidoc

zmoog · 2024-09-10T13:56:47Z

@kilfoyle, sorry for pinging you directly! 🙇

This PR introduces a breaking change in the GCP metricset. We are changing the content of the cloud.account.id and cloud.account.name fields to align them to ECS.

What do you think the best way is to communicate this change in the release notes? Do you have an example at hand we can imitate?

kilfoyle · 2024-09-10T15:28:48Z

Hi @zmoog, good question! I haven't really worked with the Beats docs but it looks like we'd need:

A new page similar to Breaking changes in 8.0. The source for that one is here.
A new bullet and new include in this Breaking changes page for whichever release will introduce the change (8.16 I guess). The source file is here. So something like this:

See the following topics for a description of breaking changes between major versions:

* <<breaking-changes-8.0>>
* <<breaking-changes-8.16>>

For breaking changes between minor versions, see the <<release-notes,release notes>>.

include::breaking-8.0.asciidoc[]
include::breaking-8.16.asciidoc[]

mergify · 2024-09-11T11:53:48Z

backport-8.x has been added to help with the transition to the new branch 8.x.

zmoog · 2024-09-13T13:53:09Z

@kilfoyle, pages like Breaking changes in 8.0 seem an excellent place for reporting breaking changes for the stack as a whole.

In this case, we want to report a breaking change in the Metricbeat's module for GCP, a humble and more limited scope.

Should we target the Metricbeat release notes? Do you think it's appropriate?

zmoog · 2024-09-13T13:54:55Z

Something like the "Breaking changes" section in 8.15.0?

kilfoyle · 2024-09-13T15:11:50Z

@zmoog Ah, yes! Sorry, I didn't realize that there are different versions of breaking changes depending on scope.

Definitely you could duplicate the breaking changes section in those 8.15 docs. So that would mean updating the 8.15.1 changelog with something like:

==== Breaking changes

*Metricbeat*

- {description}. {pull}40461[40461]

zmoog · 2024-09-13T17:02:13Z

Definitely you could duplicate the breaking changes section in those 8.15 docs. So that would mean updating the 8.15.1 changelog with something like:

Great, we'll add this section to the target release!

Thank you @kilfoyle!

zmoog

LGTM.

Before merging, please add an example of how to restore the previous behavior.

After the merge, update the integration docs when we bump the min stack version to bring this to the Elastic Agent integration.

zmoog · 2024-09-17T10:17:25Z

CHANGELOG.asciidoc

 === Beats version 8.15.1
 https://github.com/elastic/beats/compare/v8.15.0\...v8.15.1[View commits]

+==== Breaking changes
+
+*Metricbeat*
+
+- Add GCP organization and project details to ECS cloud fields. {pull}40461[40461]


This seems to be in the 8.15.1 section of the release notes, but I guess we're shipping in 8.15.2 at the earliest, right?

zmoog · 2024-09-17T10:19:48Z

metricbeat/docs/modules/gcp.asciidoc

+We have updated our field names to align with ECS semantics. As part of this change:
+
+*  `cloud.account.id` will now contain the Google Cloud Organization ID (previously, it contained the project ID).
+*  `cloud.account.name` will now contain the Google Cloud Organization Display Name (previously, it contained the project name).
+*  New fields `cloud.project.id` and `cloud.project.name` will be added to store the actual project ID and project name, respectively.
+


As @lucabelluccini suggested, could you also explain how to revert to the previous behavior for users who may depend on having the project ID in the cloud.account.* fields?

I think adding a processor in the @Custom pipeline is good enough.

We need to do the same in the Elastic Agent integrations docs.

Something like this?

[ { "set": { "field": "cloud.account.id", "value": "{{cloud.project.id}}", "if": "ctx?.cloud?.project?.id != null" } }, { "set": { "field": "cloud.account.name", "value": "{{cloud.project.name}}", "if": "ctx?.cloud?.project?.name != null" } }, { "remove": { "field": [ "cloud.project.id", "cloud.project.name" ], "ignore_missing": true } } ]

Labels with current implementation:

"cloud": { "account": { "id": "992493199029" }, "project": { "id": "elastic-observability", "name": "elastic-observability" }, },

After pipeline:

"cloud": { "project": {}, "account": { "name": "elastic-observability", "id": "elastic-observability" } },

mergify · 2024-09-17T20:51:25Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b gcp-cloud-label-fix upstream/gcp-cloud-label-fix
git merge upstream/main
git push upstream gcp-cloud-label-fix

zmoog

LGTM

… cloud labels (#40461) * issue fix * add change.log * formatting * change.log * error-fix * resolved comments * resolved comments * context added * resolve comments * error handling * doc changes * change.log * Update gcp.asciidoc * Update CHANGELOG.asciidoc * doc changes (cherry picked from commit 355ed04)

… ID and display name to cloud labels (#40899) * x-pack/metricbeat/module/gcp: Add Organization ID and display name to cloud labels (#40461) * issue fix * add change.log * formatting * change.log * error-fix * resolved comments * resolved comments * context added * resolve comments * error handling * doc changes * change.log * Update gcp.asciidoc * Update CHANGELOG.asciidoc * doc changes (cherry picked from commit 355ed04) * Update x-pack/metricbeat/module/gcp/timeseries_metadata_collector.go Co-authored-by: Harnish Chavda <[email protected]> * Update metadata.go * Update metadata.go * Update CHANGELOG.asciidoc * Update CHANGELOG.asciidoc * Update CHANGELOG.asciidoc * Delete CHANGELOG.asciidoc * Revert "Delete CHANGELOG.asciidoc" This reverts commit c1bd932. * fix changelog * Update CHANGELOG.asciidoc * Update CHANGELOG.next.asciidoc --------- Co-authored-by: Linu-Elias <[email protected]> Co-authored-by: Harnish Chavda <[email protected]> Co-authored-by: Gabriel Pop <[email protected]>

Linu-Elias added 2 commits August 8, 2024 15:59

issue fix

9d45226

add change.log

9f2752d

Linu-Elias requested review from a team as code owners August 8, 2024 11:28

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 8, 2024

mergify bot assigned Linu-Elias Aug 8, 2024

formatting

7a56bd9

Linu-Elias added the bugfix label Aug 8, 2024

Linu-Elias added 2 commits August 9, 2024 10:08

change.log

7228ac4

error-fix

be57da1

gpop63 reviewed Aug 12, 2024

View reviewed changes

x-pack/metricbeat/module/gcp/metrics/metricset.go Outdated Show resolved Hide resolved

x-pack/metricbeat/module/gcp/metrics/metricset.go Outdated Show resolved Hide resolved

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved

resolved comments

67e8aa6

Linu-Elias added 4 commits August 13, 2024 15:11

Merge branch 'main' into gcp-cloud-label-fix

8ff45bc

Merge branch 'elastic:main' into gcp-cloud-label-fix

38096b5

resolved comments

6b55a2b

context added

de598db

gpop63 reviewed Aug 21, 2024

View reviewed changes

x-pack/metricbeat/module/gcp/timeseries_metadata_collector.go Outdated Show resolved Hide resolved

kaiyan-sheng reviewed Aug 26, 2024

View reviewed changes

kaiyan-sheng approved these changes Aug 28, 2024

View reviewed changes

Merge branch 'main' into gcp-cloud-label-fix

909f6f4

zmoog reviewed Aug 30, 2024

View reviewed changes

Merge branch 'elastic:main' into gcp-cloud-label-fix

7c0e761

resolve comments

31d8ec6

zmoog approved these changes Sep 9, 2024

View reviewed changes

gpop63 reviewed Sep 9, 2024

View reviewed changes

x-pack/metricbeat/module/gcp/metrics/metricset.go Outdated Show resolved Hide resolved

mergify bot added the backport-v8.x label Sep 10, 2024

Linu-Elias added 2 commits September 10, 2024 18:11

error handling

0bff706

Merge branch 'main' into gcp-cloud-label-fix

33dc055

gpop63 reviewed Sep 10, 2024

View reviewed changes

x-pack/metricbeat/module/gcp/_meta/docs.asciidoc Outdated Show resolved Hide resolved

doc changes

06ca307

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 11, 2024

v1v removed the backport-v8.x label Sep 11, 2024

Linu-Elias added 2 commits September 17, 2024 10:28

Merge branch 'elastic:main' into gcp-cloud-label-fix

ae616bc

change.log

bef89ea

zmoog reviewed Sep 17, 2024

View reviewed changes

Linu-Elias added 2 commits September 18, 2024 18:31

Update gcp.asciidoc

5bc8500

Update CHANGELOG.asciidoc

4522f71

zmoog approved these changes Sep 18, 2024

View reviewed changes

Linu-Elias added 2 commits September 19, 2024 10:31

Merge branch 'elastic:main' into gcp-cloud-label-fix

bc83ba0

doc changes

e75868d

Linu-Elias merged commit 355ed04 into elastic:main Sep 19, 2024
29 checks passed

mergify bot mentioned this pull request Sep 19, 2024

[8.x](backport #40461) x-pack/metricbeat/module/gcp: Add Organization ID and display name to cloud labels #40899

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x-pack/metricbeat/module/gcp: Add Organization ID and display name to cloud labels #40461

x-pack/metricbeat/module/gcp: Add Organization ID and display name to cloud labels #40461

Linu-Elias commented Aug 8, 2024 •

edited

Loading

botelastic bot commented Aug 8, 2024

mergify bot commented Aug 8, 2024

mergify bot commented Aug 13, 2024

mergify bot commented Aug 23, 2024

kaiyan-sheng Aug 26, 2024

zmoog Sep 9, 2024

kaiyan-sheng left a comment

Linu-Elias commented Aug 27, 2024

zmoog left a comment

zmoog Aug 30, 2024

Linu-Elias Sep 2, 2024

lucabelluccini commented Aug 30, 2024

zmoog left a comment

zmoog Sep 9, 2024

zmoog commented Sep 10, 2024

kilfoyle commented Sep 10, 2024

mergify bot commented Sep 11, 2024

zmoog commented Sep 13, 2024

zmoog commented Sep 13, 2024

kilfoyle commented Sep 13, 2024

zmoog commented Sep 13, 2024

zmoog left a comment

zmoog Sep 17, 2024

zmoog Sep 17, 2024

gpop63 Sep 17, 2024

zmoog Sep 17, 2024

mergify bot commented Sep 17, 2024

zmoog left a comment

x-pack/metricbeat/module/gcp: Add Organization ID and display name to cloud labels #40461

x-pack/metricbeat/module/gcp: Add Organization ID and display name to cloud labels #40461

Conversation

Linu-Elias commented Aug 8, 2024 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

botelastic bot commented Aug 8, 2024

mergify bot commented Aug 8, 2024

mergify bot commented Aug 13, 2024

mergify bot commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaiyan-sheng left a comment

Choose a reason for hiding this comment

Linu-Elias commented Aug 27, 2024

zmoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucabelluccini commented Aug 30, 2024

zmoog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmoog commented Sep 10, 2024

kilfoyle commented Sep 10, 2024

mergify bot commented Sep 11, 2024

zmoog commented Sep 13, 2024

zmoog commented Sep 13, 2024

kilfoyle commented Sep 13, 2024

zmoog commented Sep 13, 2024

zmoog 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

mergify bot commented Sep 17, 2024

zmoog left a comment

Choose a reason for hiding this comment

Linu-Elias commented Aug 8, 2024 •

edited

Loading