Skip to content

JMXFetch bundled integrations disabled by default - #696

Merged
tylerbenson merged 1 commit into
masterfrom
tyler/jmxfetch-config
Feb 7, 2019
Merged

JMXFetch bundled integrations disabled by default#696
tylerbenson merged 1 commit into
masterfrom
tyler/jmxfetch-config

Conversation

@tylerbenson

Copy link
Copy Markdown
Contributor

Enable each individually by setting -Ddd.integration.<integration_name>.enabled=true.

@tylerbensontylerbenson added type: feature Enhancements and improvements inst: others All other instrumentations labels Feb 6, 2019
@tylerbensontylerbenson added this to the 0.23.0 milestone Feb 6, 2019
@tylerbenson
tylerbensonforce-pushed the tyler/jmxfetch-config branch 2 times, most recently from 8ca167d to 9e65b7cCompareFebruary 6, 2019 18:37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does the defaultEnabled have to be boxed?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This allows null to be passed in for no default.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Actually, that doesn't really work, so I'll change it to primitive.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, you cast it to primitive anyway. And besides caller should make up their mind about this default :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks odd. Did you mean to test system property and env variable separately? Otherwise it is unclear if they actually work independently. Also you may want to check that false value here works as expected.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

They are tested separately by virtue of using different values. I did add more test cases though.

@mar-kolyamar-kolya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couple of minor comments that should be addressed, otherwise LGTM

Enable each individually by setting `-Ddd.integration.<integration_name>.enabled=true`.
@tylerbenson
tylerbenson merged commit 0802bb8 into masterFeb 7, 2019
@tylerbenson
tylerbenson deleted the tyler/jmxfetch-config branch February 7, 2019 17:48
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: othersAll other instrumentationstype: featureEnhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@tylerbenson@mar-kolya