Skip to content

Fix multi-instance service discovery#3341

Merged
hkaj merged 1 commit intoDataDog:masterfrom
aerostitch:fix_multi-instance_annotations
Jun 15, 2017
Merged

Fix multi-instance service discovery#3341
hkaj merged 1 commit intoDataDog:masterfrom
aerostitch:fix_multi-instance_annotations

Conversation

@aerostitch
Copy link
Contributor

@aerostitch aerostitch commented May 12, 2017

Fixes: #3340

As explained in #3340, having multiple instances for the same check in the annotations don't work well.
This brings the few adjustments to make it work.

The doc will need to be updated to say that if you want several instances of the same check in the annotations, you'll need to have an array for then inside the annotations array.

@aerostitch
Copy link
Contributor Author

@hkaj Do you think we could setup this one for 5.14?
Thanks! :)

@aerostitch aerostitch force-pushed the fix_multi-instance_annotations branch 4 times, most recently from b672387 to dbc59a0 Compare May 12, 2017 08:09
@aerostitch
Copy link
Contributor Author

Note: I've got this running in one of my environments. Works fine with a multi-instance declared via an annotation.

@masci masci added this to the Triage milestone May 12, 2017
@aerostitch aerostitch force-pushed the fix_multi-instance_annotations branch from dbc59a0 to cfcccb7 Compare May 12, 2017 17:11
@aerostitch
Copy link
Contributor Author

Note: this will also require a fix for the page https://docs.datadoghq.com/guides/autodiscovery/
The current examples should be adjusted to be proper arrays.

@aerostitch
Copy link
Contributor Author

Hi @hkaj do you think we can plan that one for 5.14?
Thanks

Copy link
Member

@hkaj hkaj left a comment

Choose a reason for hiding this comment

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

Hi @aerostitch !
Thanks for the PR. It's an interesting feature. The freeze for 5.14 is tomorrow so it's likely going to be pushed to 5.15 though.

I left one comment, let me know what you think. Also merge conflicts need to be fixed. We'll take care of the documentation.

Thanks again!

if tpl and len(tpl) == 2:
init_config, instance = tpl
result_instances.append(instance)
result_init_config.update(init_config)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rather apply the same logic as here: https://github.com/DataDog/dd-agent/pull/3341/files#diff-8437fe4d4416c056af154d55fb2993c2R372
You can do something like

if not result_init_config:
    result_init_config = init_config
elif result_init_config != init_config:
    self.log.warning("Different versions of `init_config` found for "
        "check {}. Keeping the first one found.".format('check_name'))

This will avoid unclear errors in case init_configs diverge and values are overriden by later ones. Not sure if it can happen right now but that silent update makes me nervous. Or do you see a legit use case for it?

@aerostitch aerostitch force-pushed the fix_multi-instance_annotations branch from 03a3a9b to 3a78899 Compare May 30, 2017 19:26
@aerostitch
Copy link
Contributor Author

aerostitch commented May 30, 2017

Makes sense @hkaj for the requested change. I rebased & updated the PR waiting for travis to start the build for about 1h now.
I don't really think it's a feature, more a bug no?
If 5.14 is frozen, shouldn't we have that in 5.14.1?

@hkaj hkaj modified the milestones: 5.14.1, 5.15 May 31, 2017
@hkaj
Copy link
Member

hkaj commented May 31, 2017

Thanks, LGTM. Will merge once 5.14.x is branched off. It's more of a feature because it was explicitly designed for a single instance and that was the only configuration that was supported. Moved it to 5.14.1.

@aerostitch
Copy link
Contributor Author

Thanks a lot! :)

@hkaj hkaj merged commit a5844a8 into DataDog:master Jun 15, 2017
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