Skip to content

[service_discovery][jmx] enabling for JMX-based integrations (GRPC)#2933

Closed
truthbk wants to merge 14 commits intomasterfrom
jaime/jmx_sd
Closed

[service_discovery][jmx] enabling for JMX-based integrations (GRPC)#2933
truthbk wants to merge 14 commits intomasterfrom
jaime/jmx_sd

Conversation

@truthbk
Copy link
Member

@truthbk truthbk commented Oct 19, 2016

What does this PR do?

This PR introduces an RPC client to enable the submission of JMX configurations (in YAML format) to JMXfetch, thus enabling service discovery for JMX-based integrations (jmxfetch). The changes introduce a dependency with grpc.io. The PR might require some polishing, but is functional at this time.

Motivation

There was a fairly obvious void in the functionalities for service discovery with regard to JMX-based integrations. This PR attempts to address that issue.

Testing Guidelines

Manual, at the moment.

Additional Notes

Note that the RPC client code is generate at build time in omnibus, so this may cause some issues in terms of linting, etc... due to unfound modules.

DataDog/omnibus-software#79
DataDog/dd-agent-omnibus#96
DataDog/jmxfetch#109
DataDog/docker-dd-agent#135

@truthbk truthbk changed the title Jaime/jmx sd [service_discovery][jmx] enabling for JMX-based integrations (GRPC) Oct 19, 2016
@olivielpeau olivielpeau added this to the 5.10.0 milestone Oct 21, 2016
import signal
import sys
import time
import supervisor.xmlrpc
Copy link

Choose a reason for hiding this comment

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

nitpick but that's 3rd party

# and modify its value.
# sd_template_dir: /datadog/check_configs
#
# JMX Service Disocvery
Copy link

Choose a reason for hiding this comment

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

Do we need this flag ? Let's add some comments about why it's needed or not

self.jmx_process.terminate()

def _handle_sigreload(self, signum, frame):
# Terminate jmx process on SIGTERM signal
Copy link

Choose a reason for hiding this comment

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

Why do we need a new signal handler if we are using gRPC ?

if self.previous_config_index is None:
self.previous_config_index = config_index
return False
return True
Copy link

Choose a reason for hiding this comment

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

What's the reason for this change ?


valid_instances = True
if config['instances'] is None or not isinstance(config['instances'], list):
valid_instances = False
Copy link

Choose a reason for hiding this comment

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

You might as well raise here.

@remh remh modified the milestones: 5.11.0, 5.10.0 Nov 3, 2016
@hkaj
Copy link
Member

hkaj commented Dec 7, 2016

@truthbk can we close it?

@truthbk
Copy link
Member Author

truthbk commented Dec 21, 2016

@hkaj yes we can. Sorry about that.

@truthbk truthbk closed this Dec 21, 2016
@masci masci modified the milestones: 5.11.0, 5.12.0 Jan 24, 2017
@masci masci removed this from the 5.11.0 milestone Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants