fix!: markdown processing returns errors (#802)

* changes log message Signed-off-by: Joerg Poecher <[email protected]> * parsing markdown now returns errors; split up tests Signed-off-by: Joerg Poecher <[email protected]> * fix documentation and add data types & restrictions Signed-off-by: Joerg Poecher <[email protected]> * adds checks for duplicate keys and adds tests Signed-off-by: Joerg Poecher <[email protected]> * adds checks for invalid values on total.pass & .warning; adds tests Signed-off-by: Joerg Poecher <[email protected]> * adds more markdown tests Signed-off-by: Joerg Poecher <[email protected]> * adds integration test for markdown parsing Signed-off-by: Joerg Poecher <[email protected]> * adds more integration tests Signed-off-by: Joerg Poecher <[email protected]> * exports constants and uses them in tests Signed-off-by: Joerg Poecher <[email protected]> * adds tests for multiple markdown tiles for configuration Signed-off-by: Joerg Poecher <[email protected]> BREAKING CHANGE: parsing errors in markdown tiles for SLO configuration will now return `result=fail` in the event payload.
keptn-contrib · May 12, 2022 · e68b556 · e68b556
1 parent c93c862
commit e68b556
Show file tree

Hide file tree

Showing 10 changed files with 816 additions and 96 deletions.
diff --git a/documentation/slis-via-dashboard.md b/documentation/slis-via-dashboard.md
@@ -108,6 +108,7 @@ By default, the dynatrace-service instructs Keptn to perform the evaluation of S
 ```yaml
 comparison:
   compare_with: "single_result"
+  number_of_comparison_results: 1
   include_result_with_score: "pass"
   aggregate_function: avg
 total_score:
@@ -119,13 +120,13 @@ Further details about SLO comparison and scoring are provided in [the Keptn docu
 
 To override these defaults, add a markdown tile to the dashboard with one or more of the following `;`-separated `<key>=<value>` pairs:
 
-| Key | Description |
-|---|---|
-|`KQG.Compare.Results` | Use `<value>` as the `comparison: compare_with` value |
-|`KQG.Compare.WithScore` | Use `<value>` as the `comparison: include_result_with_score` value |
-|`KQG.Compare.Function` | Use `<value>` as the `comparison: aggregate_function` value |
-|`KQG.Total.Pass` | Use `<value>` as the `total_score: pass` value |
-|`KQG.Total.Warning` | Use `<value>` as the `total_score: warning` value |
+| Key                     | Data type (restriction)                | Description                                                                                                                                          |
+|-------------------------|----------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `KQG.Compare.Results`   | number (`> 0`)                         | Use `<value>` as the `comparison: number_of_comparison_results` value. `comparison: compare_with` will be set automatically according to this value. |
+| `KQG.Compare.WithScore` | string (`pass`, `all`, `pass_or_warn`) | Use `<value>` as the `comparison: include_result_with_score` value                                                                                   |
+| `KQG.Compare.Function`  | string (`avg`, `p50`, `p90`, `p95`)    | Use `<value>` as the `comparison: aggregate_function` value                                                                                          |
+| `KQG.Total.Pass`        | number (with optional `%`)             | Use `<value>` as the `total_score: pass` value                                                                                                       |
+| `KQG.Total.Warning`     | number (with optional `%`)             | Use `<value>` as the `total_score: warning` value                                                                                                    |
 
 For example, the defaults above could be specified using a markdown tile containing:
 

diff --git a/internal/sli/dashboard/dashboard_processing.go b/internal/sli/dashboard/dashboard_processing.go
@@ -2,6 +2,7 @@ package dashboard
 
 import (
 	"context"
+	"fmt"
 
 	keptncommon "github.com/keptn/go-utils/pkg/lib"
 	keptnv2 "github.com/keptn/go-utils/pkg/lib/v0_2_0"
@@ -47,7 +48,7 @@ func NewProcessing(client dynatrace.ClientInterface, eventData adapter.EventCont
 }
 
 // Process processes a dynatrace.Dashboard.
-func (p *Processing) Process(ctx context.Context, dashboard *dynatrace.Dashboard) *QueryResult {
+func (p *Processing) Process(ctx context.Context, dashboard *dynatrace.Dashboard) (*QueryResult, error) {
 
 	// lets also generate the dashboard link for that timeframe (gtf=c_START_END) as well as management zone (gf=MZID) to pass back as label to Keptn
 	dashboardLinkAsLabel := NewLink(p.client.Credentials().GetTenant(), p.timeframe, dashboard.ID, dashboard.GetFilter())
@@ -70,16 +71,24 @@ func (p *Processing) Process(ctx context.Context, dashboard *dynatrace.Dashboard
 		},
 	}
 
-	log.Debug("Dashboard has changed: reparsing it!")
+	log.Debug("Dashboard will be parsed!")
 
 	// now let's iterate through the dashboard to find our SLIs
+	markdownAlreadyProcessed := false
 	for _, tile := range dashboard.Tiles {
 		switch tile.TileType {
 		case dynatrace.MarkdownTileType:
-			score, comparison := NewMarkdownTileProcessing().Process(&tile, createDefaultSLOScore(), createDefaultSLOComparison())
-			if score != nil && comparison != nil {
-				result.slo.TotalScore = score
-				result.slo.Comparison = comparison
+			res, err := NewMarkdownTileProcessing().Process(&tile, createDefaultSLOScore(), createDefaultSLOComparison())
+			if err != nil {
+				return nil, fmt.Errorf("markdown tile parsing error: %w", err)
+			}
+			if res != nil {
+				if markdownAlreadyProcessed {
+					return nil, fmt.Errorf("only one markdown tile allowed for KQG configuration")
+				}
+				result.slo.TotalScore = &res.totalScore
+				result.slo.Comparison = &res.comparison
+				markdownAlreadyProcessed = true
 			}
 		case dynatrace.SLOTileType:
 			tileResults := NewSLOTileProcessing(p.client, p.timeframe).Process(ctx, &tile)
@@ -102,5 +111,5 @@ func (p *Processing) Process(ctx context.Context, dashboard *dynatrace.Dashboard
 		}
 	}
 
-	return result
+	return result, nil
 }
diff --git a/internal/sli/dashboard/dashboard_querying.go b/internal/sli/dashboard/dashboard_querying.go
@@ -38,5 +38,5 @@ func (q *Querying) GetSLIValues(ctx context.Context, dashboardID string, timefra
 		return nil, fmt.Errorf("error while processing dashboard config '%s' - %w", dashboardID, err)
 	}
 
-	return NewProcessing(q.dtClient, q.eventData, q.customSLIFilters, timeframe).Process(ctx, dashboard), nil
+	return NewProcessing(q.dtClient, q.eventData, q.customSLIFilters, timeframe).Process(ctx, dashboard)
 }
diff --git a/internal/sli/dashboard/markdown_tile_processing.go b/internal/sli/dashboard/markdown_tile_processing.go
@@ -1,12 +1,50 @@
 package dashboard
 
 import (
-	"github.com/keptn-contrib/dynatrace-service/internal/dynatrace"
-	keptncommon "github.com/keptn/go-utils/pkg/lib"
+	"fmt"
+	"regexp"
 	"strconv"
 	"strings"
+
+	keptncommon "github.com/keptn/go-utils/pkg/lib"
+
+	"github.com/keptn-contrib/dynatrace-service/internal/dynatrace"
 )
 
+type markdownParsingErrors struct {
+	errors []error
+}
+
+func (err *markdownParsingErrors) Error() string {
+	var errStrings = make([]string, len(err.errors))
+	for i, e := range err.errors {
+		errStrings[i] = e.Error()
+	}
+	return strings.Join(errStrings, ";")
+}
+
+type invalidValueError struct {
+	key   string
+	value string
+}
+
+func (err *invalidValueError) Error() string {
+	return fmt.Sprintf("invalid value for '%s': %s", err.key, err.value)
+}
+
+type duplicateKeyError struct {
+	key string
+}
+
+func (err *duplicateKeyError) Error() string {
+	return fmt.Sprintf("duplicate key '%s' in markdown definition", err.key)
+}
+
+type markdownParsingResult struct {
+	totalScore keptncommon.SLOScore
+	comparison keptncommon.SLOComparison
+}
+
 type MarkdownTileProcessing struct {
 }
 
@@ -16,61 +54,158 @@ func NewMarkdownTileProcessing() *MarkdownTileProcessing {
 }
 
 // Process will overwrite the default values for SLOScore and SLOComparison with the contents found in the markdown
-func (p *MarkdownTileProcessing) Process(tile *dynatrace.Tile, defaultScore keptncommon.SLOScore, defaultComparison keptncommon.SLOComparison) (*keptncommon.SLOScore, *keptncommon.SLOComparison) {
+func (p *MarkdownTileProcessing) Process(tile *dynatrace.Tile, defaultScore keptncommon.SLOScore, defaultComparison keptncommon.SLOComparison) (*markdownParsingResult, error) {
 	// we allow the user to use a markdown to specify SLI/SLO properties, e.g: KQG.Total.Pass
 	// if we find KQG. we process the markdown
 	return parseMarkdownConfiguration(tile.Markdown, defaultScore, defaultComparison)
 }
 
+const (
+	TotalPass                  = "kqg.total.pass"
+	TotalWarning               = "kqg.total.warning"
+	CompareWithScore           = "kqg.compare.withscore"
+	CompareWithScoreAll        = "all"
+	CompareWithScorePass       = "pass"
+	CompareWithScorePassOrWarn = "pass_or_warn"
+	CompareResults             = "kqg.compare.results"
+	CompareResultsSingle       = "single_result"
+	CompareResultsMultiple     = "several_results"
+	CompareFunction            = "kqg.compare.function"
+	CompareFunctionAvg         = "avg"
+	CompareFunctionP50         = "p50"
+	CompareFunctionP90         = "p90"
+	CompareFunctionP95         = "p95"
+)
+
 // parseMarkdownConfiguration parses a text that can be used in a Markdown tile to specify global SLO properties
-func parseMarkdownConfiguration(markdown string, totalScore keptncommon.SLOScore, comparison keptncommon.SLOComparison) (*keptncommon.SLOScore, *keptncommon.SLOComparison) {
+func parseMarkdownConfiguration(markdown string, totalScore keptncommon.SLOScore, comparison keptncommon.SLOComparison) (*markdownParsingResult, error) {
 	if !strings.Contains(markdown, "KQG.") {
 		return nil, nil
 	}
 
+	result := &markdownParsingResult{
+		totalScore: totalScore,
+		comparison: comparison,
+	}
+
+	var errs []error
+	keyFound := make(map[string]bool)
+
 	markdownSplits := strings.Split(markdown, ";")
 	for _, markdownSplitValue := range markdownSplits {
 		configValueSplits := strings.Split(markdownSplitValue, "=")
 		if len(configValueSplits) != 2 {
 			continue
 		}
 
-		// lets get configname and value
-		configName := strings.ToLower(configValueSplits[0])
-		configValue := configValueSplits[1]
-
-		switch configName {
-		case "kqg.total.pass":
-			totalScore.Pass = configValue
-		case "kqg.total.warning":
-			totalScore.Warning = configValue
-		case "kqg.compare.withscore":
-			comparison.IncludeResultWithScore = configValue
-			if (configValue == "pass") || (configValue == "pass_or_warn") || (configValue == "all") {
-				comparison.IncludeResultWithScore = configValue
-			} else {
-				comparison.IncludeResultWithScore = "pass"
+		// lets separate key and value
+		key := strings.ToLower(configValueSplits[0])
+		value := configValueSplits[1]
+
+		switch key {
+		case TotalPass:
+			if keyFound[TotalPass] {
+				errs = append(errs, &duplicateKeyError{key: TotalPass})
+				break
+			}
+			if isNotAPercentValue(value) {
+				errs = append(errs, &invalidValueError{key: TotalPass, value: value})
+			}
+			result.totalScore.Pass = value
+			keyFound[TotalPass] = true
+		case TotalWarning:
+			if keyFound[TotalWarning] {
+				errs = append(errs, &duplicateKeyError{key: TotalWarning})
+				break
 			}
-		case "kqg.compare.results":
-			noresults, err := strconv.Atoi(configValue)
+			if isNotAPercentValue(value) {
+				errs = append(errs, &invalidValueError{key: TotalWarning, value: value})
+			}
+			result.totalScore.Warning = value
+			keyFound[TotalWarning] = true
+		case CompareWithScore:
+			if keyFound[CompareWithScore] {
+				errs = append(errs, &duplicateKeyError{key: CompareWithScore})
+				break
+			}
+			score, err := parseCompareWithScore(value)
 			if err != nil {
-				comparison.NumberOfComparisonResults = 1
-			} else {
-				comparison.NumberOfComparisonResults = noresults
+				errs = append(errs, err)
 			}
-			if comparison.NumberOfComparisonResults > 1 {
-				comparison.CompareWith = "several_results"
-			} else {
-				comparison.CompareWith = "single_result"
+			result.comparison.IncludeResultWithScore = score
+			keyFound[CompareWithScore] = true
+		case CompareResults:
+			if keyFound[CompareResults] {
+				errs = append(errs, &duplicateKeyError{key: CompareResults})
+				break
+			}
+			numberOfResults, err := parseCompareNumberOfResults(value)
+			if err != nil {
+				errs = append(errs, err)
 			}
-		case "kqg.compare.function":
-			if (configValue == "avg") || (configValue == "p50") || (configValue == "p90") || (configValue == "p95") {
-				comparison.AggregateFunction = configValue
-			} else {
-				comparison.AggregateFunction = "avg"
+			result.comparison.NumberOfComparisonResults = numberOfResults
+			keyFound[CompareResults] = true
+		case CompareFunction:
+			if keyFound[CompareFunction] {
+				errs = append(errs, &duplicateKeyError{key: CompareFunction})
+				break
 			}
+			aggregateFunc, err := parseAggregateFunction(value)
+			if err != nil {
+				errs = append(errs, err)
+			}
+			result.comparison.AggregateFunction = aggregateFunc
+			keyFound[CompareFunction] = true
+		}
+	}
+
+	if len(errs) > 0 {
+		return nil, &markdownParsingErrors{
+			errors: errs,
 		}
 	}
 
-	return &totalScore, &comparison
+	result.comparison.CompareWith = CompareResultsSingle
+	if result.comparison.NumberOfComparisonResults > 1 {
+		result.comparison.CompareWith = CompareResultsMultiple
+	}
+
+	return result, nil
+}
+
+func isNotAPercentValue(value string) bool {
+	pattern := regexp.MustCompile("^(\\d+|\\d+\\.\\d+)([%]?)$")
+
+	return !pattern.MatchString(value)
+}
+
+func parseCompareWithScore(value string) (string, error) {
+	switch value {
+	case CompareWithScorePass, CompareWithScoreAll, CompareWithScorePassOrWarn:
+		return value, nil
+	}
+
+	return "", &invalidValueError{key: CompareWithScore, value: value}
+}
+
+func parseCompareNumberOfResults(value string) (int, error) {
+	numberOfResults, err := strconv.Atoi(value)
+	if err != nil {
+		return 0, &invalidValueError{key: CompareResults, value: value}
+	}
+
+	if numberOfResults < 1 {
+		return 0, &invalidValueError{key: CompareResults, value: value}
+	}
+
+	return numberOfResults, nil
+}
+
+func parseAggregateFunction(value string) (string, error) {
+	switch value {
+	case CompareFunctionAvg, CompareFunctionP50, CompareFunctionP90, CompareFunctionP95:
+		return value, nil
+	}
+
+	return "", &invalidValueError{key: CompareFunction, value: value}
 }