Skip to content

[py] Implement backward-compatible increment/decrement#623

Merged
olivielpeau merged 1 commit intomasterfrom
olivielpeau/implement-increment-decrement
Sep 26, 2017
Merged

[py] Implement backward-compatible increment/decrement#623
olivielpeau merged 1 commit intomasterfrom
olivielpeau/implement-increment-decrement

Conversation

@olivielpeau
Copy link
Copy Markdown
Member

@olivielpeau olivielpeau commented Sep 26, 2017

What does this PR do?

Implements backward-compatible increment/decrement (i.e., with same behavior as agent5)

Motivation

Some experiments on existing checks have shown that sending these
metrics with AgentCheck.count was not working well for some of the checks
(namely those that should be using gauge instead of increment/
decrement).

Until we find a reasonable deprecation path to remove these methods,
let's re-implement them fully for python checks (but keep the
deprecation warnings).

Additional notes

Follow-up to #297

Some experiments on existing checks have shown that sending these
metrics with `count` was not working well in some of the checks
(namely those that should be using `gauge` instead of `increment`/
`decrement`).

Until we find a reasonable deprecation path to remove these methods,
let's re-implement them fully for python checks (but keep the
deprecation warnings).
@olivielpeau olivielpeau added component/aggregator [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. labels Sep 26, 2017
Copy link
Copy Markdown
Contributor

@gmmeyer gmmeyer left a comment

Choose a reason for hiding this comment

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

I added one comment. It's a nit, feel free to merge or to address it and merge. I don't think we have time for nits right not 😃

@@ -86,8 +86,8 @@ def __init__(self, *args, **kwargs):
self._deprecations = {
'increment': [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add decrement here too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

actually here we log the deprecation warning once per check instance, whether increment or decrement is used in the check. Which is why there's only one entry in this dict.

The key in this dict could be renamed though (to counter maybe?) so that this behavior is more self-explanatory.

Let me know what you think, I'll go ahead and merge this but I can address your comments in a separate PR :)

@olivielpeau olivielpeau merged commit 9f3e747 into master Sep 26, 2017
@olivielpeau olivielpeau deleted the olivielpeau/implement-increment-decrement branch September 26, 2017 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/aggregator [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead..

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants