You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by sjcorbett <gi...@git.apache.org> on 2015/12/15 15:42:58 UTC
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
GitHub user sjcorbett opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1109
Republish request count sensors on poll failure
Entities that use `org.apache.brooklyn.entity.webapp.WebAppServiceMetrics#REQUEST_COUNT` are configured to republish the existing sensor value when a poll fails.
**Context**
The webapp entities (for Tomcat, JBoss, etc.) publish a request count sensor. `WebAppServiceMethods#connectWebAppServerPolicies(EntityLocal)` is used by `JavaWebAppSoftwareProcess` (among others) to enrich the sensor into values for `REQUESTS_PER_SECOND_LAST` and `REQUESTS_PER_SECOND_IN_WINDOW` sensors.
The clustered varieties of these entities (`DynamicWebAppCluster` and co.) enrich the enriched values into cluster-wide aggregates.
Enrichers are only updated when a value for the source sensor (request count) is published.
If an entity's process fails then the poll for the request count sensor fails and no value is published. As such, no values for the enriched sensors are published either and their values must be considered incorrect. This then means that policies act on stale data rather than the true state of the system.
The change in this pull request configures the handful of entities that use the `REQUEST_COUNT` sensor to republish the current value when a poll fails. Whether this is an appropriate thing to do depends on whether you think a sensor's value represents a communique that Brooklyn has learned something of the recent history of the system being monitored or that it represents something that was true in the past. (I generally assume the latter since there is no notion of timeliness when calling `sensors().get(key)`.)
An alternative approach would be to adjust the aggregating enrichers to indicate that entities whose status is not `RUNNING` should not be considered. The current design of the aggregating enrichers does not make this as straightforward as the changes in this PR.
Another would be to change `RollingTimeWindowMeanEnricher` to decay values outwith the sensor event framework.
I think this change is sufficient for the near future - it is much better than the status quo - and that long term we should choose one of the alternatives (and also that I'm probably overthinking this).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sjcorbett/incubator-brooklyn feature/republish-sensor-on-failure
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1109.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1109
----
commit 110d277924231939d48c36ccc4e4a10e84bfb7d5
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2015-12-14T16:28:35Z
Polls for REQUEST_COUNT are configured to republish current val on error
commit b4086998c676bcffa7195ef351694a9a185e61cf
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2015-12-15T14:39:11Z
Tidy
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by sjcorbett <gi...@git.apache.org>.
GitHub user sjcorbett reopened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1109
Republish request count sensors on poll failure
Entities that use `org.apache.brooklyn.entity.webapp.WebAppServiceMetrics#REQUEST_COUNT` are configured to republish the existing sensor value when a poll fails.
**Context**
The webapp entities (for Tomcat, JBoss, etc.) publish a request count sensor. `WebAppServiceMethods#connectWebAppServerPolicies(EntityLocal)` is used by `JavaWebAppSoftwareProcess` (among others) to enrich the sensor into values for `REQUESTS_PER_SECOND_LAST` and `REQUESTS_PER_SECOND_IN_WINDOW` sensors.
The clustered varieties of these entities (`DynamicWebAppCluster` and co.) enrich the enriched values into cluster-wide aggregates.
Enrichers are only updated when a value for the source sensor (request count) is published.
If an entity's process fails then the poll for the request count sensor fails and no value is published. As such, no values for the enriched sensors are published either and their values must be considered incorrect. This then means that policies act on stale data rather than the true state of the system.
The change in this pull request configures the handful of entities that use the `REQUEST_COUNT` sensor to republish the current value when a poll fails. Whether this is an appropriate thing to do depends on whether you think a sensor's value represents a communique that Brooklyn has learned something of the recent history of the system being monitored or that it represents something that was true in the past. (I generally assume the latter since there is no notion of timeliness when calling `sensors().get(key)`.)
An alternative approach would be to adjust the aggregating enrichers to indicate that entities whose status is not `RUNNING` should not be considered. The current design of the aggregating enrichers does not make this as straightforward as the changes in this PR.
Another would be to change `RollingTimeWindowMeanEnricher` to decay values outwith the sensor event framework.
I think this change is sufficient for the near future - it is much better than the status quo - and that long term we should choose one of the alternatives (and also that I'm probably overthinking this).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sjcorbett/incubator-brooklyn feature/republish-sensor-on-failure
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1109.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1109
----
commit 110d277924231939d48c36ccc4e4a10e84bfb7d5
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2015-12-14T16:28:35Z
Polls for REQUEST_COUNT are configured to republish current val on error
commit 1b7b5a831605ce734ed804ac416703542a24a8c8
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date: 2015-12-15T15:17:30Z
Tidy
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1109
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1109
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1109#issuecomment-166270746
See https://issues.apache.org/jira/browse/BROOKLYN-207.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1109#discussion_r47893741
--- Diff: core/src/main/java/org/apache/brooklyn/core/entity/EntityFunctions.java ---
@@ -154,7 +154,23 @@ protected GetEntityAttributeFunction(AttributeSensor<T> attribute) {
@Override public T apply(Entity input) {
return (input == null) ? null : input.getAttribute(attribute);
}
- };
+ }
+
+ public static <T> Function<Object, T> attribute(Entity entity, AttributeSensor<T> attribute) {
+ return new GetFixedEntityAttributeFunction<>(entity, attribute);
--- End diff --
Personal preference for `checkNotNull(entity, "entity"), checkNotNull(attribute, "attribute")`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1109#discussion_r47894122
--- Diff: software/webapp/src/test/java/org/apache/brooklyn/entity/webapp/AbstractWebAppFixtureIntegrationTest.java ---
@@ -363,42 +353,90 @@ public void publishesZeroRequestsPerSecondMetricRepeatedly(final SoftwareProcess
this.entity = entity;
log.info("test=publishesZeroRequestsPerSecondMetricRepeatedly; entity="+entity+"; app="+entity.getApplication());
- final int MAX_INTERVAL_BETWEEN_EVENTS = 4000; // TomcatServerImpl publishes events every 3000ms so this should be enough overhead
- final int NUM_CONSECUTIVE_EVENTS = 3;
+ final int maxIntervalBetweenEvents = 4000; // TomcatServerImpl publishes events every 3000ms so this should be enough overhead
--- End diff --
Bike-shedding: I quite like the upper case, treating it like a constant that is scoped to just the method. But no strong feelings.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1109#issuecomment-165426211
@sjcorbett agreed this is a good improvement / short-term fix.
I think that long-term the right philosophy is that the service-down should trigger the removal of this node from the calculations. Until that point, we don't have enough info to know if it's still working away at its previous load (in which case we don't want to zero the requests-per-second by publishing the same total-request-count again), or if it is not doing anything.
If it were genuinely not doing anything, then we should mark it as service-down.
This is obviously a fiddly thing to get right, particularly as there are several failure scenarios and there are different ways the metrics are being used.
@sjcorbett do you want to open a jira issue for us improving this long term?
Merging this now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Republish request count sensors o...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1109#issuecomment-164813629
The failed test (AbstractControllerTest.testUpdateCalledWhenChildHostnameAndPortChanges) passes for me repeatedly when I run locally. I have opened [BROOKLYN-206](https://issues.apache.org/jira/browse/BROOKLYN-206).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---