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

x-pack/metricbeat/module/gcp: Override GCP API endpoint in metric client #40918

Merged
merged 5 commits into from
Sep 21, 2024

Conversation

Linu-Elias
Copy link
Contributor

@Linu-Elias Linu-Elias commented Sep 20, 2024

Proposed commit message

The GCP metric client does not currently allow overriding with a custom endpoint, which is necessary for testing against a mock GCP API endpoint.
Resolved this issue by implementing a check to determine if an endpoint is specified in the GCP configuration, and subsequently overriding the ClientOption with that endpoint.

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

Use cases

Screenshots

Logs

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 20, 2024
Copy link
Contributor

mergify bot commented Sep 20, 2024

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-8./d is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Sep 20, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 20, 2024
@Linu-Elias Linu-Elias marked this pull request as ready for review September 20, 2024 12:18
@Linu-Elias Linu-Elias requested review from a team as code owners September 20, 2024 12:18
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Sep 20, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 20, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

Copy link
Collaborator

@pierrehilbert pierrehilbert left a comment

Choose a reason for hiding this comment

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

Approving for the data plane side as we are only owning the yml files.

@pierrehilbert pierrehilbert added Team:obs-ds-hosted-services Label for the Observability Hosted Services team Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team labels Sep 20, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

Comment on lines 110 to 115
Endpoint string `config:"endpoint"`
opt []option.ClientOption
period *durationpb.Duration
organizationID string
organizationName string
projectName string
Copy link
Contributor

@gpop63 gpop63 Sep 20, 2024

Choose a reason for hiding this comment

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

Let's keep a new line between these exported and unexported struct fields like it was before.

Suggested change
Endpoint string `config:"endpoint"`
opt []option.ClientOption
period *durationpb.Duration
organizationID string
organizationName string
projectName string
Endpoint string `config:"endpoint"`
opt []option.ClientOption
period *durationpb.Duration
organizationID string
organizationName string
projectName string

@@ -59,6 +59,7 @@ https://github.com/elastic/beats/compare/v8.8.1\...main[Check the HEAD diff]
- Mark system process metricsets as running if metrics are partially available {pull}40565[40565]
- Added back `elasticsearch.node.stats.jvm.mem.pools.*` to the `node_stats` metricset {pull}40571[40571]
- Add GCP organization and project details to ECS cloud fields. {pull}40461[40461]
- Add `endpoint` field to the GCP configuration in order to append it to the metric client. {issue}40848[40848] {pull}40918[40918]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Add `endpoint` field to the GCP configuration in order to append it to the metric client. {issue}40848[40848] {pull}40918[40918]
- Add support for specifying a custom endpoint for GCP service clients. {issue}40848[40848] {pull}40918[40918]

Comment on lines 73 to 81
- module: gcp
metricsets:
- compute
region: "us-"
project_id: "your project id"
credentials_file_path: "your JSON credentials file path"
endpoint: http://your-endpoint
exclude_labels: false
period: 1m
Copy link
Contributor

Choose a reason for hiding this comment

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

The yaml block is not properly formatted and it might be redundant to add it again just to showcase the endpoint config param. I would probably add endpoint to an existing config in that file.

Comment on lines +146 to +149
if m.config.Endpoint != "" {
m.Logger().Warnf("You are using a custom endpoint '%s' for the GCP API calls.", m.config.Endpoint)
m.config.opt = append(m.config.opt, option.WithEndpoint(m.config.Endpoint))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new line to improve readability.

Suggested change
if m.config.Endpoint != "" {
m.Logger().Warnf("You are using a custom endpoint '%s' for the GCP API calls.", m.config.Endpoint)
m.config.opt = append(m.config.opt, option.WithEndpoint(m.config.Endpoint))
}
if m.config.Endpoint != "" {
m.Logger().Warnf("You are using a custom endpoint '%s' for the GCP API calls.", m.config.Endpoint)
m.config.opt = append(m.config.opt, option.WithEndpoint(m.config.Endpoint))
}

@Linu-Elias Linu-Elias merged commit 7720c90 into elastic:main Sep 21, 2024
29 checks passed
mergify bot pushed a commit that referenced this pull request Sep 21, 2024
…ent (#40918)

* override endpoint to etric client

* config and changelog

* doc

* resolved comments

* lint fix

(cherry picked from commit 7720c90)
pierrehilbert pushed a commit that referenced this pull request Sep 23, 2024
… endpoint in metric client (#40938)

* x-pack/metricbeat/module/gcp:  Override GCP API endpoint in metric client (#40918)

* override endpoint to etric client

* config and changelog

* doc

* resolved comments

* lint fix

(cherry picked from commit 7720c90)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Linu-Elias <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:obs-ds-hosted-services Label for the Observability Hosted Services team Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GCP] Metricset metrics client allow overriding the endpoint
5 participants