Replace host metadata in dogstasd payload with host tag #66
Conversation
|
cc @remh - what do you think of this approach? |
olivielpeau
force-pushed
the
olivielpeau/replace-hostname-in-sc
branch
from
67113c0
to
348f70d
Compare
olivielpeau
force-pushed
the
olivielpeau/replace-hostname-in-sc
branch
from
348f70d
to
d3a2a16
Compare
| @@ -494,7 +491,7 @@ public void testServiceCheckCounter() throws Exception { | |||
| // Let's put a service check in the pipeline (we cannot call doIteration() | |||
| // here unfortunately because it would call reportStatus which will flush | |||
| // the count to the jmx_status.yaml file and reset the counter. | |||
| repo.sendServiceCheck("jmx", Status.STATUS_OK, "This is a test", "jmx_test_instance", null); | |||
| repo.sendServiceCheck("jmx", Status.STATUS_OK, "This is a test", null); | |||
|
|
|||
| // Let's check that the counter has been updated | |||
| assertEquals(1, repo.getServiceCheckCount("jmx")); | |||
There was a problem hiding this comment.
Can we test the jmx_server:hostname tag ? In the meantime, maybe we can start moving those service checks tests in a different file to improve the readibility.
There was a problem hiding this comment.
Hmm actually all of our tests rely on the Attach API and thus don't use a host in the configuration, so we can't cleanly test the jmx_server tag with our current test setup...
We could change our test setup to use JMX Remote instead but that would need some work.
There was a problem hiding this comment.
Oh that's right.
Switching to JMX Remote is not worth. A better solution would be to be able to unit test the specific getServiceCheckTags method, and not instantiate the whole app like we are currently doing. This is what we do for instance with TestConfiguration.
Let's see if it can be done easily and decide.
|
Looks good to me 🍰. We should discuss the wording |
The host name that's sent is the host that's defined in the yaml config for each instance, which is preventing the backend from correctly assigning host tags to the service checks. We fix that by sending a `jmx_server` tag in the dogstatsd payload instead of a host_name metadata field. The agent's dogstatsd will then add the agent's hostname. Also removed unused variables in `TestApp`.
olivielpeau
force-pushed
the
olivielpeau/replace-hostname-in-sc
branch
from
d3a2a16
to
9da215d
Compare
|
Thanks @yannmh for the review! I've removed the |
|
LGTM @olivielpeau . Can you merge and build a new jar for the |
|
I've tested the new jar on staging and it works fine: host tags are available during the setup of new monitors on JMXFetch service checks. |
|
No feedback from the customer unfortunately but merging anyway. |
|
👍 |
Replace host metadata in dogstasd payload with host tag
Supersedes #65.
The host name that's sent is the host that's defined in the yaml config for each instance, which is preventing the backend from correctly assigning host tags to the service checks.
We fix that by sending a
jmx_servertag in the dogstatsd payload instead of a host_name metadata field. The agent's dogstatsd will then add the agent's hostname.Also added an
is_jmxtag to JMXFetch's service checks so that the backend can know which service checks come from JMXFetch.cc @talwai @conorbranagan