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 check prefix config #284

Merged
merged 5 commits into from
Mar 27, 2020

Conversation

AlexandreYang
Copy link
Member

@AlexandreYang AlexandreYang commented Mar 16, 2020

Add option to configure the prefix used for service checks.

This solves two issues:

@AlexandreYang AlexandreYang force-pushed the alex/jmxfetch_add_service_check_prefix_config branch from 15abfdf to a4a17db Compare March 16, 2020 17:42
@AlexandreYang AlexandreYang marked this pull request as ready for review March 16, 2020 17:56
@AlexandreYang AlexandreYang changed the title Add check prefix Add check prefix config Mar 16, 2020
@AlexandreYang AlexandreYang requested a review from a team March 25, 2020 10:57
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

This change makes sense overall 👍 Made a few suggestions inline

src/main/java/org/datadog/jmxfetch/Instance.java Outdated Show resolved Hide resolved
import org.apache.commons.lang.StringUtils;

public class ServiceCheckHelper {
/** Formats the service check prefix. */
Copy link
Member

Choose a reason for hiding this comment

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

Let's add an explanation here about what this formatting does, and why, see explanation here: 0428c41

This was, in hindsight, a poor solution to the technical problem at the time. As a way to start the transition to not using this formatting logic anymore, maybe it'd make sense to send both the formatted service check name and the unformatted one when your new option check_prefix is not specified, what do you think?

Copy link
Member Author

@AlexandreYang AlexandreYang Mar 25, 2020

Choose a reason for hiding this comment

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

Formats the service check prefix name to enable having versionned .yaml
configuration files for the same checks (ex. : activemq/activemq_58)
while keeping on sending the same metric prefix. All numbers,
underscores and some other characters are simply removed from the prefix
if they are present.

I'm still not sure to understand why there was a need to strip those characters based on this comment ☝️ in order to have versioning (I don't have much context on this versioning concept).

This was, in hindsight, a poor solution to the technical problem at the time. As a way to start the transition to not using this formatting logic anymore, maybe it'd make sense to send both the formatted service check name and the unformatted one when your new option check_prefix is not specified, what do you think?

Yes, make sense. So, if the check_name is foo_bar:

  • If check_prefix = foo, we send 2 service checks foo.can_connect and foobar.can_connect
  • If check_prefix is not set, we send 2 service checks foo_can.connect and foobar.can_connect
  • We deprecate the current formatting: foobar.can_connect

But then we need a deprecation process for when we should remove the old behaviour ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm still not sure to understand why there was a need to strip those characters based on this comment ☝️ in order to have versioning (I don't have much context on this versioning concept).

at the time there were 2 JMXFetch integrations for activemq: one called activemq for older versions of activemq, the other called activemq_58 for v5.8+ of activemq, see https:/DataDog/dd-agent/tree/5.10.x/conf.d

And we still wanted both integrations to send the service check with the activemq prefix, hence this formatting function...

So, if the check_name is foo_bar:

  • If check_prefix = foo, we send 2 service checks foo.can_connect and foobar.can_connect
  • If check_prefix is not set, we send 2 service checks foo_can.connect and foobar.can_connect
  • We deprecate the current formatting: foobar.can_connect

I agree overall. In the case of check_prefix = foo, it's up to you whether we still want to send foobar.can_connect (we should if you need that behavior for existing integrations where you'll start using check_prefix, otherwise we should only send foo.can_connect)

But then we need a deprecation process for when we should remove the old behaviour ?

Yes, unclear how we'll do that. But at least JMXFetch will already be sending the "right" service check name.

Copy link
Member Author

@AlexandreYang AlexandreYang Mar 26, 2020

Choose a reason for hiding this comment

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

@olivielpeau Just made changes related to this. Let me know if it's ok.

Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@AlexandreYang AlexandreYang merged commit c25dcc3 into master Mar 27, 2020
@prognant prognant deleted the alex/jmxfetch_add_service_check_prefix_config branch April 2, 2020 11:40
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.

3 participants