Skip to content

Support pluggable custodian monitor#3299

Merged
jaydoane merged 1 commit into3.xfrom
pluggable-custodian-monitor
Dec 15, 2020
Merged

Support pluggable custodian monitor#3299
jaydoane merged 1 commit into3.xfrom
pluggable-custodian-monitor

Conversation

@jaydoane
Copy link
Contributor

Overview

Enable build time configurable monitor for custodian and remove custom
sensu events.

Testing recommendations

make eunit apps=custodian

Related Issues or Pull Requests

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

Copy link
Member

@rnewson rnewson left a comment

Choose a reason for hiding this comment

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

Almost :)



-callback send_missing_db_alert(DbName :: binary()) ->
Output :: string().
Copy link
Member

Choose a reason for hiding this comment

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

I'd encode the fact that we don't use the result of these functions, we call them only for their implementation. so perhaps insist on false atom in all cases or say any() and document that the result of the function is ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in fixup



send_missing_db_alert(_DbName) ->
"no-op".
Copy link
Member

Choose a reason for hiding this comment

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

adjust these to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{ok, nil}.

check_shards() ->
[send_sensu_event(Item) || Item <- custodian:summary()].
Copy link
Member

Choose a reason for hiding this comment

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

this is a cut too far imo. we still want to log the descriptive error message to couch_log. the pluggable module allows a sysadmin to send that message (or one of its own invention) to some other system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, I didn't even notice that logging cleverly interleaved in the case statement. In any case, I've returned them, and changed the send_event/3 callback to DRY things up a bit.

couch_log:warning("~s", [Description]),
"--warning"
end,
?CUSTODIAN_MONITOR:send_event(Name, Level, Description).
Copy link
Member

Choose a reason for hiding this comment

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

think we should send Count itself, the transformation to Level is sensu-specific. 😬

Copy link
Member

@rnewson rnewson left a comment

Choose a reason for hiding this comment

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

nice! please squash before merging!

Enable build time configurable monitor for custodian and remove custom
sensu events.
@jaydoane jaydoane force-pushed the pluggable-custodian-monitor branch from 2496735 to 313ef4b Compare December 15, 2020 22:15
@jaydoane jaydoane merged commit c0754a6 into 3.x Dec 15, 2020
@jaydoane jaydoane deleted the pluggable-custodian-monitor branch December 15, 2020 22:46
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.

2 participants