Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store outcome values in servicemetrics transaction.success_count #9791

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Store outcome values in servicemetrics transaction.success_count #9791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2105468
Store aggregated outcome in servicemetrics transaction.success_count
carsonip Dec 12, 2022
aa4c5a0
Remove `transaction.failure_count`
carsonip Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions apmpackage/apm/changelog.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
- description: Enable synthetic source for metrics data streams
type: enhancement
link: https://github.com/elastic/apm-server/pull/9756
- description: Remove `transaction.failure_count` and change `transaction.success_count` type to aggregate_metric_double
type: enhancement
link: https://github.com/elastic/apm-server/pull/9791
- version: "8.6.0"
changes:
- description: Change `ecs.version` to a `constant_keyword` field
Expand Down
6 changes: 0 additions & 6 deletions apmpackage/apm/data_stream/internal_metrics/fields/fields.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,6 @@
type: keyword
description: |
Keyword of specific relevance in the service's domain (eg: 'db.postgresql.query', 'template.erb', 'cache', etc).
- name: transaction.failure_count
type: long
description: "Count of transactions with 'event.outcome: failure'"
- name: transaction.success_count
type: long
description: "Count of transactions with 'event.outcome: success'"
- name: transaction.duration.histogram
type: histogram
description: |
Expand Down
4 changes: 4 additions & 0 deletions apmpackage/apm/data_stream/internal_metrics/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ elasticsearch:
type: aggregate_metric_double
metrics: [sum, value_count]
default_metric: sum
success_count:
type: aggregate_metric_double
metrics: [sum, value_count]
default_metric: sum
settings:
index:
sort.field: "@timestamp"
Expand Down
1 change: 1 addition & 0 deletions changelogs/head.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ https://github.com/elastic/apm-server/compare/8.5\...main[View commits]
- `context.http.response.*_size` fields now enforce integer values {pull}9429[9429]
- `observer.id` and `observer.ephemeral_id` are no longer added to APM documents {pull}9412[9412]
- `timeseries.instance` has been removed from transaction metrics docs; it was never used {pull}9565[9565]
- `transaction.failure_count` has been removed. `transaction.success_count` type has changed to `aggregated_metric_double` {pull}9791[9791]

[float]
==== Deprecations
Expand Down
2 changes: 2 additions & 0 deletions internal/model/modeldecoder/v2/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,8 @@ func TestDecodeMapToTransactionModel(t *testing.T) {
"DurationSummary.Sum",
"FailureCount",
"SuccessCount",
"SuccessCount.Count",
"SuccessCount.Sum",
"Root":
return true
}
Expand Down
29 changes: 7 additions & 22 deletions internal/model/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,23 +60,11 @@ type Transaction struct {
// is subject to removal.
DurationSummary SummaryMetric

// FailureCount holds an aggregated count of transactions with the
// outcome "failure". If FailureCount is zero, it will be omitted from
// the output event.
//
// NOTE(axw) this is used only for service metrics, which are in technical
// preview. Do not use this field without discussion, as the field mapping
// is subject to removal.
FailureCount int

// SuccessCount holds an aggregated count of transactions with the
// outcome "success". If SuccessCount is zero, it will be omitted from
// the output event.
//
// NOTE(axw) this is used only for service metrics, which are in technical
// preview. Do not use this field without discussion, as the field mapping
// is subject to removal.
SuccessCount int
// SuccessCount holds an aggregated count of transactions with different
// outcomes. A "failure" adds to the Count. A "success" adds to both the
// Count and the Sum. An "unknown" has no effect. If Count is zero, it
// will be omitted from the output event.
SuccessCount SummaryMetric

Marks TransactionMarks
Message *Message
Expand Down Expand Up @@ -112,11 +100,8 @@ func (e *Transaction) fields() mapstr.M {
if e.DurationSummary.Count != 0 {
transaction.maybeSetMapStr("duration.summary", e.DurationSummary.fields())
}
if e.FailureCount != 0 {
transaction.set("failure_count", e.FailureCount)
}
if e.SuccessCount != 0 {
transaction.set("success_count", e.SuccessCount)
if e.SuccessCount.Count != 0 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should success_count be omitted from output if Count is zero? e.g. when it only contains unknown outcome transactions.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is not omitted on count=0, transaction.success_count will have to be a pointer to SummaryMetric, otherwise any event that contains the transaction field will have transaction.success_count.sum: 0 and transaction.success_count.count: 0

Copy link
Member

Choose a reason for hiding this comment

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

I think omitting when count is zero is the way to go.

transaction.maybeSetMapStr("success_count", e.SuccessCount.fields())
}
transaction.maybeSetString("name", e.Name)
transaction.maybeSetString("result", e.Result)
Expand Down
10 changes: 8 additions & 2 deletions systemtest/approvals/TestServiceMetricsAggregation.approved.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@
"value_count": 2
}
},
"success_count": 2,
"success_count": {
"sum": 2,
"value_count": 2
},
"type": "type1"
}
},
Expand Down Expand Up @@ -84,7 +87,10 @@
"value_count": 2
}
},
"success_count": 2,
"success_count": {
"sum": 2,
"value_count": 2
},
"type": "type2"
}
}
Expand Down
8 changes: 5 additions & 3 deletions x-pack/apm-server/aggregation/servicemetrics/aggregator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,9 +372,11 @@ func makeMetricset(key aggregationKey, metrics serviceMetrics) model.APMEvent {
Name: metricsetName,
},
Transaction: &model.Transaction{
Type: key.transactionType,
FailureCount: int(math.Round(metrics.failureCount)),
SuccessCount: int(math.Round(metrics.successCount)),
Type: key.transactionType,
SuccessCount: model.SummaryMetric{
Count: int64(math.Round(metrics.successCount + metrics.failureCount)),
Sum: metrics.successCount,
},
DurationSummary: model.SummaryMetric{
Count: metricCount,
Sum: float64(time.Duration(math.Round(metrics.transactionDuration)).Microseconds()),
Expand Down
11 changes: 8 additions & 3 deletions x-pack/apm-server/aggregation/servicemetrics/aggregator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,10 @@ func TestAggregatorRun(t *testing.T) {
Count: 6,
Sum: 6000, // 6ms in micros
},
SuccessCount: 2,
FailureCount: 3,
SuccessCount: model.SummaryMetric{
Count: 5,
Sum: 2,
},
},
}, {
Processor: model.MetricsetProcessor,
Expand Down Expand Up @@ -279,7 +281,10 @@ func TestAggregatorMaxGroups(t *testing.T) {
Count: 1,
Sum: 100000,
},
SuccessCount: 1,
SuccessCount: model.SummaryMetric{
Count: 1,
Sum: 1,
},
},
Metricset: &model.Metricset{Name: "service", DocCount: 1},
}, m)
Expand Down