-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
Codecov Report
@@ 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 Report
@@ 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 Report
@@ 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried it? 😝
There was a problem hiding this comment.
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 😉
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried it? 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline 👍
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 add_environment
fixture. For example:would become:
This doesn't impact
local
E2E because the host level Agent already includes JMXFetchMotivation
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)
changelog/
andintegration/
labels attached