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

[mapreduce] Consolidate tags #2474

Merged

Conversation

zachradtka
Copy link
Contributor

This is mainly a bugfix to reduce the number of tags collected with the MapReduce agent check.

The major fixes are:

  • Commenting out the counter options in the YAML
  • Tag metrics with cluster_name instead of cluster_id

@olivielpeau olivielpeau self-assigned this May 6, 2016
@olivielpeau olivielpeau added this to the 5.8.0 milestone May 6, 2016
@gmmeyer
Copy link
Contributor

gmmeyer commented May 6, 2016

Thank you very much for your contribution!

We'll review and test this very soon, but it looks pretty good. The failing test looks unrelated, so I wouldn't worry about that.

# Get the cluster name from the conf file
cluster_name = instance.get('cluster_name')
if cluster_name is None:
raise Exception('The cluster_name must be specified in the instance configuration')
Copy link
Member

Choose a reason for hiding this comment

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

same comment as the YARN check, it'd be better to add a warning and set a default value instead of raising an exception.

@olivielpeau
Copy link
Member

@zachradtka Looks pretty good, only had one comment similar to my suggestion on the yarn PR.

@zachradtka
Copy link
Contributor Author

@olivielpeau I have updated the check and tests to remove the INFO_URL and to ensure that a warning is displayed if the user does not specify a cluster_name in the YAML

@olivielpeau
Copy link
Member

Thanks @zachradtka, looks good, can you squash your commits please?

@zachradtka zachradtka force-pushed the minerkasch/mapreduce-consolidate-tags branch from 67d0208 to 36d37a5 Compare May 9, 2016 20:58
@olivielpeau
Copy link
Member

Thanks! Merging

@olivielpeau olivielpeau merged commit f713250 into DataDog:master May 9, 2016
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