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

[corechecks] Support custom tags in every corechecks #2723

Merged
merged 18 commits into from
Jan 23, 2019

Conversation

remeh
Copy link
Contributor

@remeh remeh commented Nov 29, 2018

What does this PR do?

This PR unify the use of custom tags in corechecks configuration files.
Some corechecks were correctly using the tags field of their configuration file, but they did it manually, some others corechecks were completely ignoring it.

Motivation

Be consistent with the work done by agent-integrations which made sure that the tags configuration field is correctly used.

Additional Notes

While a check is running CheckBase.CommonConfigure(...), if the tags configuration field is set, those tags are stored into the sender used by the current check, copying the behavior of the fields min_collection_interval and empty_default_hostname.
Thus, when the sender is used to send a metric, an event or a service check, it appends the custom tags to the message.

I've chosen this solution mainly for those pros:

  • the checks developer can't forget them while sending metrics, etc.
  • the next check using CheckBase won't need to manually deal with the tags configuration field
  • no flag to set on checks or anything in the code by the developer. The sole presence of tags inside the corechecks configuration activate this behavior.

But it has cons:

  • it's kind of implicit for the developer of new corechecks, which is something I tend to dislike. But this is this implicit part which avoids the developer to forget to add custom tags while sending metrics, events, etc
  • the checkSender is responsible for storing the custom tags, which is ok, but not ideal.
  • I had to remove code from the checks which were manually dealing with custom tags, same goes for some of their unit/integration tests, making this PR more verbose.

Some design choices:

  • the check doesn't have access to its custom tags → avoid the mistake of adding them again while sending metrics, etc.

This is mainly used to attach tags read in checks configuration
to an instanciated sender.

Those custom tags will be appended to each send (event, metric, service).
Some checks were already appending their custom tags to the metrics,
events and services check sent.

This commit removes this as it's now done by the `checkSender`.
@remeh remeh added kind/enhancement do-not-merge/WIP [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. changelog/no-changelog labels Nov 29, 2018
@remeh remeh added this to the Triage milestone Nov 29, 2018
@remeh remeh requested a review from a team November 29, 2018 11:07
@remeh remeh requested a review from a team as a code owner November 29, 2018 11:07
@remeh remeh changed the title Support custom tags for corechecks [corechecks] Support custom tags in every corechecks Nov 30, 2018
@codecov-io
Copy link

codecov-io commented Nov 30, 2018

Codecov Report

Merging #2723 into master will increase coverage by 0.03%.
The diff coverage is 45.61%.

@@            Coverage Diff             @@
##           master    #2723      +/-   ##
==========================================
+ Coverage    57.4%   57.44%   +0.03%     
==========================================
  Files         392      393       +1     
  Lines       25616    25765     +149     
==========================================
+ Hits        14705    14800      +95     
- Misses       9943     9988      +45     
- Partials      968      977       +9
Impacted Files Coverage Δ
pkg/collector/corechecks/system/disk.go 68.83% <ø> (+3.77%) ⬆️
pkg/collector/corechecks/containers/cri.go 27.9% <ø> (-1.64%) ⬇️
...g/collector/corechecks/containers/docker_events.go 68.75% <ø> (+0.56%) ⬆️
pkg/autodiscovery/integration/config.go 55.03% <ø> (ø) ⬆️
pkg/collector/corechecks/containers/docker.go 6.04% <0%> (+0.02%) ⬆️
pkg/aggregator/mocksender/mocked_methods.go 14.81% <0%> (-1.19%) ⬇️
pkg/collector/corechecks/checkbase.go 32.83% <0%> (-3.84%) ⬇️
pkg/aggregator/mocksender/mocksender.go 93.54% <100%> (+0.21%) ⬆️
pkg/collector/corechecks/net/network.go 49.23% <100%> (-0.77%) ⬇️
pkg/collector/corechecks/system/disk_nix.go 73.01% <100%> (-1.23%) ⬇️
... and 10 more

@@ -42,7 +42,7 @@ type DockerConfig struct {
CollectImageSize bool `yaml:"collect_image_size"`
CollectDiskStats bool `yaml:"collect_disk_stats"`
CollectVolumeCount bool `yaml:"collect_volume_count"`
Tags []string `yaml:"tags"`
Tags []string `yaml:"tags"` // Used only by the configuration converter v5 → v6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this attribute only because it's used by the legacy converter is not great. But for now, I've no good solution except this comment to avoid somebody to use this field.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to be bullet-proof, you can use anonymous embedding to create a new type extending DockerConfig, in the legacy package:

type fullDockerConfig struct {
    containers.DockerConfig
    Tags                     []string           `yaml:"tags"`
}

But your approach is OK

hush-hush
hush-hush previously approved these changes Dec 26, 2018
Copy link
Member

@hush-hush hush-hush left a comment

Choose a reason for hiding this comment

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

👍

pkg/collector/corechecks/checkbase.go Outdated Show resolved Hide resolved
pkg/collector/corechecks/checkbase.go Outdated Show resolved Hide resolved
@@ -42,7 +42,7 @@ type DockerConfig struct {
CollectImageSize bool `yaml:"collect_image_size"`
CollectDiskStats bool `yaml:"collect_disk_stats"`
CollectVolumeCount bool `yaml:"collect_volume_count"`
Tags []string `yaml:"tags"`
Tags []string `yaml:"tags"` // Used only by the configuration converter v5 → v6
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to be bullet-proof, you can use anonymous embedding to create a new type extending DockerConfig, in the legacy package:

type fullDockerConfig struct {
    containers.DockerConfig
    Tags                     []string           `yaml:"tags"`
}

But your approach is OK

Copy link
Contributor

@xvello xvello left a comment

Choose a reason for hiding this comment

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

LGTM, please get a review from croissant too

@remeh remeh merged commit 12cf59d into master Jan 23, 2019
@albertvaka albertvaka deleted the remeh/corechecks-custom-tags branch July 8, 2019 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. kind/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants