[yarn] Add configurable application tags #2489
Conversation
|
@olivielpeau this should improve the recently merged PR #2473 |
| for tag in app_tags: | ||
| key, value = tag.split(':') | ||
| tags.append("{tag}:{value}".format( | ||
| tag=value, value=str(app_json[key]) |
There was a problem hiding this comment.
If app_json[key] doesn't exist, this will raise a KeyError, failing the check.
I think it would be better to either test the existence of the value before appending it or try/catch it. (and emit a log.warning without failing if there is an issue)
|
@alanbover Thanks for the PR, I added a few comments, looks good otherwise ! |
|
@degemer I've pushed again after some fix after your comments. Much appreciated! |
| try: | ||
| key, value = tag.split(':') | ||
| except ValueError: | ||
| self.log.error("Invalid value {0} for application_tag").format() |
There was a problem hiding this comment.
Typo ? Should probably be something like:
self.log.error("Invalid value {0} for application_tag".format(tag)
|
Added a few comments, should be good after that. |
Added application tags in the configuration file, allowing set extra tags for app metrics like queue, user, etc. Also, added the tags functionality merged in #2473 on tests
|
Hi, Thanks for your comments, I've missed that! I've pushed the changes |
| tag=value, value=str(app_json[key]) | ||
| )) | ||
| except ValueError: | ||
| self.log.error("Invalid value {0} for application_tag").format(tag) |
There was a problem hiding this comment.
The parenthesis is still not correct 😉 .
Also could you use the logger interpolation instead of doing it with format ? https://www.simonmweber.com/2014/11/24/python-logging-traps.html
It should be self.log.error("Invalid value %s for application_tag", tag).
|
@alanbover There is still a small typo, 👍 after that. |
|
@alanbover Were you by any chance able to review the typos @degemer mentioned? |
|
Closed in favor of #3041 |
Added application tags in the configuration file,
allowing set extra tags for app metrics like queue,
user, etc.
Also, added the tags functionality merged in
#2473
on tests