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

Add ability to detect if using JMX based on metadata #3330

Merged
merged 3 commits into from
Mar 20, 2019
Merged

Conversation

nmuesch
Copy link
Collaborator

@nmuesch nmuesch commented Mar 20, 2019

What does this PR do?

Adds use_jmx to the metadata for E2E to allow the command to automatically use the jmx agent container if its needed.

To use this on an existing check, its as simple as modifying the yield statement in a dd_environment fixture. For example:

yield common.BASIC_CONFIG

would become:

yield common.BASIC_CONFIG, {'use_jmx': True}

This doesn't impact local E2E because the host level Agent already includes JMXFetch

Motivation

When testing JMX based integrations with E2E it's required to always pass in the -a flag and use the JMX based agent container. This will auto detect that and allow us to specify in the fixture whether the check should default to using the JMX container.

Additional Notes

If the -a flag is passed, the change made in this PR are ignored.

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@nmuesch nmuesch requested a review from a team as a code owner March 20, 2019 10:18
@nmuesch nmuesch changed the title Add ability to detect if using based on metadata Add ability to detect if using JMX based on metadata Mar 20, 2019
@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #3330 into master will decrease coverage by 8.2%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3330      +/-   ##
==========================================
- Coverage   85.96%   77.75%   -8.21%     
==========================================
  Files         705       31     -674     
  Lines       37047     1043   -36004     
  Branches     4469       93    -4376     
==========================================
- Hits        31846      811   -31035     
+ Misses       3976      200    -3776     
+ Partials     1225       32    -1193

2 similar comments
@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #3330 into master will decrease coverage by 8.2%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3330      +/-   ##
==========================================
- Coverage   85.96%   77.75%   -8.21%     
==========================================
  Files         705       31     -674     
  Lines       37047     1043   -36004     
  Branches     4469       93    -4376     
==========================================
- Hits        31846      811   -31035     
+ Misses       3976      200    -3776     
+ Partials     1225       32    -1193

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #3330 into master will decrease coverage by 8.2%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3330      +/-   ##
==========================================
- Coverage   85.96%   77.75%   -8.21%     
==========================================
  Files         705       31     -674     
  Lines       37047     1043   -36004     
  Branches     4469       93    -4376     
==========================================
- Hits        31846      811   -31035     
+ Misses       3976      200    -3776     
+ Partials     1225       32    -1193

@@ -96,6 +98,9 @@ def start(ctx, check, env, agent, dev, base, env_vars):
else:
agent_build = agent_ver.get(env_type, env_type)

if not isinstance(agent_ver, string_types) and use_jmx:
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its so that if a user specified -a we don't add -jmx to the end 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried it? 😝

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have, works like a charm 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -96,6 +98,9 @@ def start(ctx, check, env, agent, dev, base, env_vars):
else:
agent_build = agent_ver.get(env_type, env_type)

if not isinstance(agent_ver, string_types) and use_jmx:
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried it? 😝

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Discussed offline 👍

@nmuesch nmuesch merged commit 6876c7b into master Mar 20, 2019
@nmuesch nmuesch deleted the nick/jmx_autouse branch March 20, 2019 17:20
@nmuesch nmuesch mentioned this pull request Mar 20, 2019
6 tasks
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.

3 participants