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

[cassandra] YAML configuration template versioning #263

Merged
merged 3 commits into from Apr 24, 2016

Conversation

yannmh
Copy link
Member

@yannmh yannmh commented Dec 10, 2015

[cassandra] YAML configuration template versioning

  • Enhance DatadogMonitor resource with a version attribute.
  • Use version attribute in cassandra recipe to version, i.e.
    select the appriopriate YAML configuration file.
    Two versions are currently available
    • 1 (Default): Legacy YAML configuration file, compatible with
      Cassandra < 2.2.
    • 2: Required for Cassandra > 2.2, use the YAML configuration file introduced with
      DataDog/dd-agent@2df7566
  • Enable cassandra_aliasing option to comply with
    [jmxfetch][cassandra] 0.9.0 + enable CASSANDRA-4009 support  dd-agent#2035

More information:
DataDog/dd-agent#2142

This is quite an important change, would you mind taking a pass on it @remh, @miketheman please ?

Thanks

@miketheman
Copy link
Contributor

@yannmh You can likely run rake style:ruby:auto_correct to handle 90% of the reported style issues.

@yannmh
Copy link
Member Author

yannmh commented Dec 11, 2015

Thanks @miketheman. I fixed the offenses 🎨. and updated the PR.

@yannmh yannmh added this to the Next minor milestone Jan 8, 2016
@yannmh yannmh removed their assignment Jan 13, 2016
@yannmh yannmh changed the title [cassandra] is_version_greater_22 to select YAML [cassandra] use_new_metrics to select YAML Jan 13, 2016
@yannmh yannmh changed the title [cassandra] use_new_metrics to select YAML [cassandra] YAML configuration template versioning Jan 14, 2016
@yannmh
Copy link
Member Author

yannmh commented Jan 14, 2016

Brought the last changes discussed @remh. I'll merge it after i get your 👍

@remh
Copy link
Contributor

remh commented Jan 14, 2016

LGTM, what's the best way to document this ?

@yannmh
Copy link
Member Author

yannmh commented Jan 14, 2016

@remh I think the current way to do it is by adding relevant comments in the recipe itself 😏 , c.f. https://github.com/DataDog/chef-datadog/blob/master/recipes/cassandra.rb#L3-L26.
@miketheman could you confirm this point please ?

@yannmh yannmh force-pushed the yann/cassandra-greater-22 branch 2 times, most recently from 786f308 to 3b3da0f Compare January 15, 2016 17:03
@yannmh
Copy link
Member Author

yannmh commented Jan 15, 2016

Added some documentation.

@yannmh yannmh changed the title [cassandra] YAML configuration template versioning [WIP][cassandra] YAML configuration template versioning Jan 19, 2016
@yannmh yannmh changed the title [WIP][cassandra] YAML configuration template versioning [cassandra] YAML configuration template versioning Jan 19, 2016
@remh
Copy link
Contributor

remh commented Jan 21, 2016

LGTM but @miketheman should give the final 👍

@miketheman
Copy link
Contributor

Thanks! We're planning a merge-a-thon next week.

@miketheman
Copy link
Contributor

Getting back to this now.

@yannmh There seems to be a slight mismatch between the current dd-agent example and the erb template here.

Here's a diff:

       Expected equivalent JSON
       Diff:


       @@ -46,18 +46,13 @@
              {
                "exclude": {
                  "keyspace": [
       -            "OpsCenter",
                    "system",
                    "system_auth",
                    "system_distributed",
       -            "system_schema",
                    "system_traces"
                  ]
                },
                "include": {
       -          "bean_regex": [
       -            ".*keyspace=.*"
       -          ],
                  "domain": "org.apache.cassandra.metrics",
                  "name": [
                    "TotalDiskSpaceUsed",
  1. The OpsCenter keyspace appears in dd-agent's example config exclusion - should it be excluded?
  2. The system_schema was excluded from dd-agent in [cassandra] exclude new system_schema keyspace dd-agent#2339 - this is not reflected in this pull yet.
  3. The bean_regex is from [cassandra] Exclude aggregated metrics not associated with a specific keyspace dd-agent#2271 - should this be applied to the current template, or only to the version 2 template?

Items 1 and 2 were released with Agent 5.7.0, Item 3 is pending Agent 5.7.4 release.

@miketheman
Copy link
Contributor

Pinging @yannmh & @sethrosenblum for follow-up questions in last comment.

@sethrosenblum
Copy link
Contributor

sethrosenblum commented Apr 21, 2016

It looks to me like all these things should be carried over to this PR. @johnaxel can you confirm that? question 3 looks like it should only apply to version 2.

@johnaxel
Copy link

Agreed on all counts @sethrosenblum.

@sethrosenblum
Copy link
Contributor

@miketheman 🎾

@miketheman
Copy link
Contributor

@sethrosenblum Thanks! I believe this branch needs a rebase against master to pick up the current test suite.

yannmh and others added 2 commits April 22, 2016 13:52
* Enhance `DatadogMonitor` resource with a `version` attribute.
* Use `version` attribute in `cassandra` recipe to version, i.e.
  select the appriopriate YAML configuration file.
  Two versions are currently available
  * `1` (Default): Legacy YAML configuration file, compatible with
    Cassandra < 2.2.
  * `2`: Required for Cassandra > 2.2, use the YAML configuration file
  * introduced with
    DataDog/dd-agent@2df7566
* Enable `cassandra_aliasing` option to comply with
  DataDog/dd-agent#2035

More information:
DataDog/dd-agent#2142
* Exclude metrics from Datastax Opscenter
* Exclude metrics from system_schema as per DataDog/dd-agent#2339
* Exclude keyspace metrics ColumnFamily metrics not associated with a keyspace as per DataDog/dd-agent#2271
Covers both version 1 (default) and version 2 template configurations.
@miketheman miketheman merged commit f084639 into master Apr 24, 2016
@miketheman
Copy link
Contributor

Thanks @sethrosenblum and @yannmh ! I've added a spec test to cover these templates, and may remove the kitchen tests in the next minor version.

@sethrosenblum sethrosenblum deleted the yann/cassandra-greater-22 branch April 25, 2016 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants