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

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

Merged
merged 7 commits into from
Jul 28, 2021

Conversation

endorama
Copy link
Member

@endorama endorama commented Jul 19, 2021

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
Collaborator

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
Collaborator

elasticmachine commented Jul 19, 2021

💚 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

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.

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
Member Author

endorama commented Jul 20, 2021

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
Copy link
Member Author

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
@kaiyan-sheng
Copy link
Contributor

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
Copy link
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
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.

Missing a changelog?

@mergify
Copy link
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 added a commit that referenced this pull request Jul 28, 2021
…override (#27094)

Co-authored-by: endorama <526307+endorama@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.15.0 Automated backport with mergify enhancement Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants