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

[wmi] extend tagging for related class & constants #1388

Merged
merged 2 commits into from
Mar 14, 2015

Conversation

yannmh
Copy link
Member

@yannmh yannmh commented Feb 20, 2015

Rebased #1253 on 5.2.0 agent release.

Added a few changes:

Thanks a lot @rlaveycal

@yannmh yannmh self-assigned this Feb 20, 2015
@yannmh yannmh added this to the 5.3.0 milestone Feb 20, 2015
@yannmh yannmh force-pushed the yann/wmi-extend-tagging branch from 88ea2f3 to 33423cd Compare February 20, 2015 01:10
# `constant_tags` optionally lets you tag each metric with a set of fixed values
#
# `link_tag` optionally specifies how to join to another property for tagging.
# Settings this will cause any instance number to be removed from tag_by values
Copy link
Member

Choose a reason for hiding this comment

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

typo Setting

@LeoCavaille
Copy link
Member

Thanks Yann, looking good!

Just two nitpicky comments, to make it look even nicer:

  • can you make the commit messages follow these guidelines
    • short title are great to see in a git log (< 72 chars)
    • flags, like [wmi] are easy to grep for
  • can you keep the same spacing conventions in the YAML file it
    should be only 2 spaces comments

@remh remh assigned LeoCavaille and yannmh and unassigned yannmh and LeoCavaille Feb 23, 2015
@yannmh yannmh force-pushed the yann/wmi-extend-tagging branch from 33423cd to 9316166 Compare February 23, 2015 15:55
@yannmh
Copy link
Member Author

yannmh commented Feb 23, 2015

Thanks @LeoCavaille !

@yannmh yannmh changed the title Yann/wmi extend tagging [wmi] extend tagging for related class & constants Feb 27, 2015
@remh remh assigned LeoCavaille and unassigned yannmh Feb 27, 2015
@LeoCavaille
Copy link
Member

Tried it on my box @yannmh , works well.

  • One thing I am slightly worried about is the number of tags your example can create, on my simple box with nothing running on it: ~40 commandline and ~100 name tags.
    Would this be possible to comment all examples in the sample file and let the user uncomment what he really wants?
    Can we also remove the allapps example, it's not helpful?
  • Second remark: looks like we cannot use several link_tag lists? Do you think it's a valid usecase?

@LeoCavaille
Copy link
Member

Also, maybe reword the comment for link_tag

 # `link_tag` optionally specifies how to join to another property for tagging.
 # Setting this will cause any instance number to be removed from tag_by values
 # i.e. name:process#1 => name:process
 # `link_tag` is a list of four parameters: [param1, param2, param3, param4]
 #     param1: property of source that contains the link value
 #     param2: class to link to
 #     param3: property of target class to link to
 #     param4: property of target class that contains the value to tag with

@yannmh yannmh assigned yannmh and unassigned LeoCavaille Mar 13, 2015
@yannmh yannmh force-pushed the yann/wmi-extend-tagging branch 4 times, most recently from d30d9b4 to b720e9b Compare March 13, 2015 20:05
@yannmh
Copy link
Member Author

yannmh commented Mar 13, 2015

Thanks @LeoCavaille. I brought some changes following your feedback.

I am merging it when Travis tests are passed,

@yannmh yannmh force-pushed the yann/wmi-extend-tagging branch from b720e9b to d1af6ea Compare March 14, 2015 16:41
yannmh added a commit that referenced this pull request Mar 14, 2015
[wmi] extend tagging for related class & constants
@yannmh yannmh merged commit bb723d0 into master Mar 14, 2015
@yannmh yannmh deleted the yann/wmi-extend-tagging branch March 14, 2015 18:02
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d1af6ea on yann/wmi-extend-tagging into * on master*.

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.

4 participants