This repository was archived by the owner on Dec 21, 2023. It is now read-only.
#4079 support timeframe property in lighthouse service#4083
Merged
Conversation
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Codecov Report
@@ Coverage Diff @@
## master #4083 +/- ##
==========================================
- Coverage 43.64% 43.14% -0.50%
==========================================
Files 236 236
Lines 12322 12265 -57
Branches 348 348
==========================================
- Hits 5378 5292 -86
- Misses 6244 6286 +42
+ Partials 700 687 -13
Continue to review full report at Codecov.
|
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Comment on lines
+148
to
+156
| params := timeutils.GetStartEndTimeParams{ | ||
| StartDate: e.Evaluation.Start, | ||
| EndDate: e.Evaluation.End, | ||
| Timeframe: e.Evaluation.Timeframe, | ||
| } | ||
| start, end, err := timeutils.GetStartEndTime(params) | ||
| if err != nil { | ||
| return "", "", err | ||
| } |
There was a problem hiding this comment.
What happens when timeframe is set, but start and end are not set?
I think in issue #4079 I got it wrong, what I meant was that
end = evaluation.triggered.time, and start = end - timeframe
Member
Author
There was a problem hiding this comment.
when only the time frame is set, we adapt the same logic we did previously in the evaluation endpoint: end = time.Now(), start = time.Now() - timeframe
There was a problem hiding this comment.
The big difference here is that the evaluation endpoint is called in a specific moment.
The cloudevent is sent at a specific moment, but the time it is retrieved by lighthouse could be a different time.
Hence I would propose to not use time.Now(), but the actual cloudevents triggered time (when was the evaluation triggered).
I know it doesn't make much difference in our current setup (maybe a couple of milliseconds), but using the cloudevents time would make the behaviour reproducible and deterministic.
Think about re-triggering an evaluation.
# Conflicts: # cli/go.mod # cli/go.sum # cli/internal/cespec/data.go
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Contributor
|
The following Docker Images have been built:
|
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4079
Closes #4091
This PR also fixes a problem that caused the
triggeredidof a<stage>.<sequence>.finishedevent to not be set properly when there are multiple iterations of the same sequence.Signed-off-by: Florian Bacher florian.bacher@dynatrace.com
Remediation with one iteration:
Note: The
warningresult is caused by a modification I did to the SLO.yaml in order to provoke another iterationRemediation with two iterations:
Integration test run: https://github.com/keptn/keptn/actions/runs/852286068