You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2015/02/02 17:09:15 UTC
[GitHub] incubator-brooklyn pull request: fix time-sensitive sensor derivat...
GitHub user ahgittin opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/501
fix time-sensitive sensor derivation when rebinding to old sensors
before this, autoscaler might scale out on rebind, because the
last total reqs might be 100, if data is v old, current value might be 10000;
a recent timestamp is attached to initial value, meaning it computes 9900 new reqs in a 1s window.
this forces an invalid timestamp for enrichers
better fix would be to store timestamp on sensors themselves
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/incubator-brooklyn fix-delayed-rebind-time-sensors
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/501.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 #501
----
commit ed04d6795cb0727334f32844bfa043608c5bd4f7
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2015-02-02T12:37:55Z
fix time-sensitive sensor derivation when rebinding to old sensors
before this, autoscaler might scale out on rebind, because the
last total reqs might be 100, if data is v old, current value might be 10000;
a recent timestamp is attached to initial value, meaning it computes 9900 new reqs in a 1s window.
this forces an invalid timestamp for enrichers
better fix would be to store timestamp on sensors themselves
----
---
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: fix time-sensitive sensor derivat...
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/501#discussion_r24329386
--- Diff: policy/src/main/java/brooklyn/enricher/RollingTimeWindowMeanEnricher.java ---
@@ -103,31 +119,69 @@ public void onEvent(SensorEvent<T> event) {
public void onEvent(SensorEvent<T> event, long eventTime) {
values.addLast(event.getValue());
timestamps.addLast(eventTime);
- pruneValues(eventTime);
- entity.setAttribute((AttributeSensor<Double>)target, getAverage(eventTime).value); //TODO this can potentially go stale... maybe we need to timestamp as well?
+ if (eventTime>0) {
+ ConfidenceQualifiedNumber average = getAverage(eventTime, 0);
+
+ if (average.confidence > getConfig(CONFIDENCE_REQUIRED_TO_PUBLISH)) {
+ // without confidence, we might publish wildly varying estimates,
+ // causing spurious resizes, so allow it to be configured, and
+ // by default require a high value
+
+ // TODO would be nice to include timestamp, etc
+ entity.setAttribute((AttributeSensor<Double>)target, average.value);
+ }
+ }
}
public ConfidenceQualifiedNumber getAverage() {
--- End diff --
Nothing seems to call the `getAverage()` method, so the `TIMESTAMP_GRACE_TIME` is never used. Or am I missing something?
---
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: fix time-sensitive sensor derivat...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/501#issuecomment-73036352
@aledsage @grkvlt can you look at this PR also? without it, odd things happen on rebind with policies.
---
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: fix time-sensitive sensor derivat...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/501#discussion_r24336185
--- Diff: policy/src/main/java/brooklyn/enricher/RollingTimeWindowMeanEnricher.java ---
@@ -103,31 +119,69 @@ public void onEvent(SensorEvent<T> event) {
public void onEvent(SensorEvent<T> event, long eventTime) {
values.addLast(event.getValue());
timestamps.addLast(eventTime);
- pruneValues(eventTime);
- entity.setAttribute((AttributeSensor<Double>)target, getAverage(eventTime).value); //TODO this can potentially go stale... maybe we need to timestamp as well?
+ if (eventTime>0) {
+ ConfidenceQualifiedNumber average = getAverage(eventTime, 0);
+
+ if (average.confidence > getConfig(CONFIDENCE_REQUIRED_TO_PUBLISH)) {
+ // without confidence, we might publish wildly varying estimates,
+ // causing spurious resizes, so allow it to be configured, and
+ // by default require a high value
+
+ // TODO would be nice to include timestamp, etc
+ entity.setAttribute((AttributeSensor<Double>)target, average.value);
+ }
+ }
}
public ConfidenceQualifiedNumber getAverage() {
- return getAverage(System.currentTimeMillis());
+ return getAverage(System.currentTimeMillis(), getConfig(TIMESTAMP_GRACE_TIME).toMilliseconds());
}
- public ConfidenceQualifiedNumber getAverage(long now) {
- pruneValues(now);
+ public ConfidenceQualifiedNumber getAverage(long fromTimeExact) {
+ return getAverage(fromTimeExact, 0);
+ }
+
+ public ConfidenceQualifiedNumber getAverage(long fromTime, long graceAllowed) {
if (timestamps.isEmpty()) {
return lastAverage = new ConfidenceQualifiedNumber(lastAverage.value, 0.0d);
}
+
+ // (previously there was an old comment here, pre-Jul-2014,
--- End diff --
In that case I'll take it out
---
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: fix time-sensitive sensor derivat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/501
---
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: fix time-sensitive sensor derivat...
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/501#discussion_r24328969
--- Diff: policy/src/main/java/brooklyn/enricher/RollingTimeWindowMeanEnricher.java ---
@@ -103,31 +119,69 @@ public void onEvent(SensorEvent<T> event) {
public void onEvent(SensorEvent<T> event, long eventTime) {
values.addLast(event.getValue());
timestamps.addLast(eventTime);
- pruneValues(eventTime);
- entity.setAttribute((AttributeSensor<Double>)target, getAverage(eventTime).value); //TODO this can potentially go stale... maybe we need to timestamp as well?
+ if (eventTime>0) {
+ ConfidenceQualifiedNumber average = getAverage(eventTime, 0);
+
+ if (average.confidence > getConfig(CONFIDENCE_REQUIRED_TO_PUBLISH)) {
+ // without confidence, we might publish wildly varying estimates,
+ // causing spurious resizes, so allow it to be configured, and
+ // by default require a high value
+
+ // TODO would be nice to include timestamp, etc
+ entity.setAttribute((AttributeSensor<Double>)target, average.value);
+ }
+ }
}
public ConfidenceQualifiedNumber getAverage() {
- return getAverage(System.currentTimeMillis());
+ return getAverage(System.currentTimeMillis(), getConfig(TIMESTAMP_GRACE_TIME).toMilliseconds());
}
- public ConfidenceQualifiedNumber getAverage(long now) {
- pruneValues(now);
+ public ConfidenceQualifiedNumber getAverage(long fromTimeExact) {
+ return getAverage(fromTimeExact, 0);
+ }
+
+ public ConfidenceQualifiedNumber getAverage(long fromTime, long graceAllowed) {
if (timestamps.isEmpty()) {
return lastAverage = new ConfidenceQualifiedNumber(lastAverage.value, 0.0d);
}
+
+ // (previously there was an old comment here, pre-Jul-2014,
--- End diff --
Include a `TODO` in this comment - it's the sort of comment we want to delete if/when we figure out what the old comment meant, or we're happy with it. Hence it justifies a TODO because we want to change it.
---
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: fix time-sensitive sensor derivat...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/501#issuecomment-72503483
took a while to get all the tests happy with this change but finally there
---
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: fix time-sensitive sensor derivat...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/501#issuecomment-73554351
thanks @aledsage; i've implemented your suggestions, apart from `Propagator` where I've now added a TODO.
you are right in your thinking that we want this for `Propagator`; I simply hadn't noticed it. it was `Transformer` that was causing the errors i was encountering. however fixing it for propagator seems a little less clear-cut, and i've not tested it, so all i have done is the todo.
I think to fix, we need to make a timestamp a first-class property of sensors.
okay to merge as changed?
---
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: fix time-sensitive sensor derivat...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/501#issuecomment-73516427
@ahgittin I'm not sure I follow the fix.
Is the main fix that `BasicSensorEvent` now defaults to the current timestamp, rather than 0?
I think I follow the change to `Transformer`, but what about `Propagator` where it calls `emitAllAttributes` when first setting all entities? Would the same logic as for Transformer not apply for Propagator as well?
---
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: fix time-sensitive sensor derivat...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/501#issuecomment-73773786
Looks good; merging.
---
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.
---