Skip to content

[Metricbeat] gcp: allow service metric prefix override#26960

Merged
endorama merged 7 commits intoelastic:masterfrom
endorama:gcp-refactor-metric-prefix
Jul 28, 2021
Merged

[Metricbeat] gcp: allow service metric prefix override#26960
endorama merged 7 commits intoelastic:masterfrom
endorama:gcp-refactor-metric-prefix

Conversation

@endorama
Copy link
Copy Markdown
Contributor

@endorama endorama commented Jul 19, 2021 •
edited
Loading

What does this PR do?

While working at #26824 I found out that some GCP Stackdriver metrics require a prefix which is not in the form <servicename>.googleapis.com.

This PR address this by adding a new field in the manifest.yml: service_metric_prefix. If this field is set, the value is used as metric prefix; if is not set, the previous behaviour is kept for backward compatibility.

Provide tests for the old and new behaviours.

Why is it important?

This change is required for #26824

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.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Scenario: Set required value for GCP metric collection service prefix
	When a developer adds a new GCP metricset
	Given service_metric_prefix is filled
	Then service_metric_prefix value is used as metric prefix

Scenario: Empty value for GCP metric collection service prefix
	When a developer adds a new GCP metricset
	Given service_metric_prefix is left empty
	Then service_metric_prefix is computed using the provided service name

Screenshots

Logs

@endorama endorama added enhancement Team:Integrations Label for the Integrations team backport-v7.15.0 Automated backport with mergify labels Jul 19, 2021
@endorama endorama requested a review from kaiyan-sheng July 19, 2021 16:21
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 19, 2021

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jul 19, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 19, 2021 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-27T15:05:47.681+0000

  • Duration: 110 min 48 sec

  • Commit: 1444fa9

Test stats 🧪

Test Results
Failed 0
Passed 9417
Skipped 2517
Total 11934

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 9417
Skipped 2517
Total 11934

kaiyan-sheng
kaiyan-sheng reviewed Jul 19, 2021
View reviewed changes
Copy link
Copy Markdown
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.

One question here: when both service_metric_prefix and service are given, service will be ignored?

Also adding a new config option also requires updating the documentation here: https://github.com/elastic/beats/blob/master/x-pack/metricbeat/module/gcp/_meta/docs.asciidoc

@endorama
Copy link
Copy Markdown
Contributor Author

endorama commented Jul 20, 2021 •
edited
Loading

One question here: when both service_metric_prefix and service are given, service will be ignored?

@kaiyan-sheng yes, as the explicit configuration takes priority.

We may want to deprecate the current logic based on service and require all manifests to be explicit about their metric prefix (:+1: from me on this). This is the smallest change to allow moving on GKE integration without introducing any breaking change or deprecation.

@endorama endorama mentioned this pull request Jul 20, 2021
Merged
6 tasks
@endorama
Copy link
Copy Markdown
Contributor Author

endorama commented Jul 20, 2021

Also adding a new config option also requires updating the documentation here: master/x-pack/metricbeat/module/gcp/_meta/docs.asciidoc

@kaiyan-sheng I added it to the manifest.yml so, from my understanding, is not a "config" option (as in "metricbeat config").
In the asciidoc files there is no mention of other manifest related options so I'm not sure where to add it.

@endorama endorama changed the title gcp: allow service metric prefix override [Metricbeat] gcp: allow service metric prefix override Jul 20, 2021
@endorama endorama mentioned this pull request Jul 20, 2021
Merged
6 tasks
@kaiyan-sheng
Copy link
Copy Markdown
Contributor

kaiyan-sheng commented Jul 20, 2021

Also adding a new config option also requires updating the documentation here: master/x-pack/metricbeat/module/gcp/_meta/docs.asciidoc

@kaiyan-sheng I added it to the manifest.yml so, from my understanding, is not a "config" option (as in "metricbeat config").
In the asciidoc files there is no mention of other manifest related options so I'm not sure where to add it.

Since user has the freedom to use metrics metricset in gcp, this config should also be exposed under metrics (on top of adding into manifest.yml. So probably two things to add/adjust here:

  1. In default config file
  2. In metrics metricset specific documentation

@endorama endorama requested a review from kaiyan-sheng July 21, 2021 15:55
jsoriano
jsoriano reviewed Jul 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Nice to see this done in a backwards compatible way 👍

Added some suggestions about the new setting, feel free to ignore them, not sure if I have all the context 🙂

@endorama endorama force-pushed the gcp-refactor-metric-prefix branch from 1c3c0e1 to d29a101 Compare July 22, 2021 09:25
@endorama endorama self-assigned this Jul 26, 2021
kaiyan-sheng
kaiyan-sheng reviewed Jul 27, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Missing a changelog?

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jul 27, 2021

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-refactor-metric-prefix upstream/gcp-refactor-metric-prefix
git merge upstream/master
git push upstream gcp-refactor-metric-prefix

@endorama endorama force-pushed the gcp-refactor-metric-prefix branch from 844a055 to 1444fa9 Compare July 27, 2021 15:05
@endorama endorama merged commit 5481b06 into elastic:master Jul 28, 2021
@endorama endorama deleted the gcp-refactor-metric-prefix branch July 28, 2021 08:06
mergify bot pushed a commit that referenced this pull request Jul 28, 2021
@endorama
[Metricbeat] gcp: allow service metric prefix override (#26960)
8634c0a 
(cherry picked from commit 5481b06)
@mergify mergify bot mentioned this pull request Jul 28, 2021
Merged
endorama added a commit that referenced this pull request Jul 28, 2021
@mergify @endorama
[7.x](backport #26960) [Metricbeat] gcp: allow service metric prefix …
f6703ef 
…override (#27094)

Co-authored-by: endorama <526307+endorama@users.noreply.github.com>
This was referenced Aug 9, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsoriano jsoriano jsoriano left review comments

+1 more reviewer

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

Reviewers whose approvals may not affect merge requirements

Assignees

@endorama endorama

Labels

backport-v7.15.0 Automated backport with mergify enhancement Team:Integrations Label for the Integrations team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@endorama @elasticmachine @kaiyan-sheng @jsoriano