Skip to content

Commit

Permalink
aggregation/txmetrics: drop user-agent parsing (#4439) (#4513)
Browse files Browse the repository at this point in the history 
RUM-specific charts have been removed from the
APM app, and these metrics are not currently
required by the User Experience app.
  • Loading branch information
@axw
axw authored Dec 14, 2020
1 parent 87b5fbf commit 85b27f9
Show file tree
Hide file tree
Showing 10 changed files with 23 additions and 221 deletions.
3 changes: 0 additions & 3 deletions beater/config/aggregation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ const (
defaultTransactionAggregationInterval = time.Minute
defaultTransactionAggregationMaxGroups = 10000
defaultTransactionAggregationHDRHistogramSignificantFigures = 2
defaultTransactionAggregationRUMUserAgentLRUSize = 5000

defaultServiceDestinationAggregationInterval = time.Minute
defaultServiceDestinationAggregationMaxGroups = 10000
Expand All @@ -43,7 +42,6 @@ type TransactionAggregationConfig struct {
Interval time.Duration `config:"interval" validate:"min=1"`
MaxTransactionGroups int `config:"max_groups" validate:"min=1"`
HDRHistogramSignificantFigures int `config:"hdrhistogram_significant_figures" validate:"min=1, max=5"`
RUMUserAgentLRUSize int `config:"rum.user_agent.lru_size" validate:"min=1"`
}

// ServiceDestinationAggregationConfig holds configuration related to span metrics aggregation for service maps.
Expand All @@ -59,7 +57,6 @@ func defaultAggregationConfig() AggregationConfig {
Interval: defaultTransactionAggregationInterval,
MaxTransactionGroups: defaultTransactionAggregationMaxGroups,
HDRHistogramSignificantFigures: defaultTransactionAggregationHDRHistogramSignificantFigures,
RUMUserAgentLRUSize: defaultTransactionAggregationRUMUserAgentLRUSize,
},
ServiceDestinations: ServiceDestinationAggregationConfig{
Enabled: true,
Expand Down
34 changes: 13 additions & 21 deletions beater/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,6 @@ func TestUnpackConfig(t *testing.T) {
"interval": "1s",
"max_groups": 123,
"hdrhistogram_significant_figures": 1,
"rum": map[string]interface{}{
"user_agent": map[string]interface{}{
"lru_size": 123,
},
},
},
"service_destinations": map[string]interface{}{
"max_groups": 456,
Expand Down Expand Up @@ -222,7 +217,6 @@ func TestUnpackConfig(t *testing.T) {
Interval: time.Second,
MaxTransactionGroups: 123,
HDRHistogramSignificantFigures: 1,
RUMUserAgentLRUSize: 123,
},
ServiceDestinations: ServiceDestinationAggregationConfig{
Enabled: true,
Expand Down Expand Up @@ -273,12 +267,11 @@ func TestUnpackConfig(t *testing.T) {
},
},
},
"jaeger.grpc.enabled": true,
"api_key.enabled": true,
"aggregation.transactions.enabled": true,
"aggregation.transactions.rum.user_agent.lru_size": 123,
"aggregation.service_destinations.enabled": false,
"sampling.keep_unsampled": false,
"jaeger.grpc.enabled": true,
"api_key.enabled": true,
"aggregation.transactions.enabled": true,
"aggregation.service_destinations.enabled": false,
"sampling.keep_unsampled": false,
"sampling.tail": map[string]interface{}{
"enabled": true,
"interval": "2m",
Expand Down Expand Up @@ -358,7 +351,6 @@ func TestUnpackConfig(t *testing.T) {
Interval: time.Minute,
MaxTransactionGroups: 10000,
HDRHistogramSignificantFigures: 2,
RUMUserAgentLRUSize: 123,
},
ServiceDestinations: ServiceDestinationAggregationConfig{
Enabled: false,
Expand Down Expand Up @@ -456,33 +448,33 @@ func TestTLSSettings(t *testing.T) {
"ConfiguredToRequired": {
config: map[string]interface{}{"ssl": map[string]interface{}{
"client_authentication": "required",
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
}},
tls: &tlscommon.ServerConfig{ClientAuth: 4, Certificate: testdataCertificateConfig},
},
"ConfiguredToOptional": {
config: map[string]interface{}{"ssl": map[string]interface{}{
"client_authentication": "optional",
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
}},
tls: &tlscommon.ServerConfig{ClientAuth: 3, Certificate: testdataCertificateConfig},
},
"DefaultRequiredByCA": {
config: map[string]interface{}{"ssl": map[string]interface{}{
"certificate_authorities": []string{"../../testdata/tls/ca.crt.pem"},
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
}},
tls: &tlscommon.ServerConfig{ClientAuth: 4, Certificate: testdataCertificateConfig},
},
"ConfiguredWithCA": {
config: map[string]interface{}{"ssl": map[string]interface{}{
"client_authentication": "none",
"certificate_authorities": []string{"../../testdata/tls/ca.crt.pem"},
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
"key": "../../testdata/tls/key.pem",
"certificate": "../../testdata/tls/certificate.pem",
}},
tls: &tlscommon.ServerConfig{ClientAuth: 0, Certificate: testdataCertificateConfig},
},
Expand Down
1 change: 0 additions & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ require (
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.6.1
github.com/t-yuki/gocover-cobertura v0.0.0-20180217150009-aaee18c8195c
github.com/ua-parser/uap-go v0.0.0-20200325213135-e1c09f13e2fe
github.com/uber/tchannel-go v1.16.0 // indirect
github.com/urso/magetools v0.0.0-20200125210132-c2e338f92f3a // indirect
github.com/xeipuuv/gojsonschema v0.0.0-20181112162635-ac52e6811b56
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -963,8 +963,6 @@ github.com/tsg/go-daemon v0.0.0-20200207173439-e704b93fd89b/go.mod h1:jAqhj/JBVC
github.com/tsg/gopacket v0.0.0-20200626092518-2ab8e397a786 h1:B/IVHYiI0d04dudYw+CvCAGqSMq8d0yWy56eD6p85BQ=
github.com/tsg/gopacket v0.0.0-20200626092518-2ab8e397a786/go.mod h1:RIkfovP3Y7my19aXEjjbNd9E5TlHozzAyt7B8AaEcwg=
github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM=
github.com/ua-parser/uap-go v0.0.0-20200325213135-e1c09f13e2fe h1:aj/vX5epIlQQBEocKoM9nSAiNpakdQzElc8SaRFPu+I=
github.com/ua-parser/uap-go v0.0.0-20200325213135-e1c09f13e2fe/go.mod h1:OBcG9bn7sHtXgarhUEb3OfCnNsgtGnkVf41ilSZ3K3E=
github.com/uber-go/atomic v1.4.0/go.mod h1:/Ct5t2lcmbJ4OSe/waGBoaVvVqtO0bmtfVNex1PFV8g=
github.com/uber/jaeger-client-go v2.15.0+incompatible/go.mod h1:WVhlPFC8FDjOFMMWRy2pZqQJSXxYSwNYOkTr/Z6d3Kk=
github.com/uber/jaeger-client-go v2.16.0+incompatible h1:Q2Pp6v3QYiocMxomCaJuwQGFt7E53bPYqEgug/AoBtY=
Expand Down
20 changes: 6 additions & 14 deletions systemtest/aggregation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,23 +55,15 @@ func TestTransactionAggregation(t *testing.T) {
require.NoError(t, err)

// Send some transactions to the server to be aggregated.
//
// Mimic a RUM transaction by using the "page-load" transaction type,
// which causes user-agent to be parsed and included in the aggregation
// and added to the document fields.
tracer := srv.Tracer()
const chromeUserAgent = "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.125 Safari/537.36"
for _, transactionType := range []string{"backend", "page-load"} {
tx := tracer.StartTransaction("name", transactionType)
req, _ := http.NewRequest("GET", "/", nil)
req.Header.Set("User-Agent", chromeUserAgent)
tx.Context.SetHTTPRequest(req)
tx.Duration = time.Second
tx.End()
}
tx := tracer.StartTransaction("name", "backend")
req, _ := http.NewRequest("GET", "/", nil)
tx.Context.SetHTTPRequest(req)
tx.Duration = time.Second
tx.End()
tracer.Flush(nil)

result := systemtest.Elasticsearch.ExpectMinDocs(t, 2, "apm-*",
result := systemtest.Elasticsearch.ExpectDocs(t, "apm-*",
estest.ExistsQuery{Field: "transaction.duration.histogram"},
)
systemtest.ApproveEvents(t, t.Name(), result.Hits.Hits, "@timestamp")
Expand Down
56 changes: 0 additions & 56 deletions systemtest/approvals/TestTransactionAggregation.approved.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,62 +52,6 @@
"root": true,
"type": "backend"
}
},
{
"@timestamp": "dynamic",
"agent": {
"name": "go"
},
"ecs": {
"version": "dynamic"
},
"event": {
"ingested": "dynamic",
"outcome": "unknown"
},
"host": {
"hostname": "beowulf",
"name": "beowulf"
},
"observer": {
"ephemeral_id": "dynamic",
"hostname": "dynamic",
"id": "dynamic",
"type": "apm-server",
"version": "dynamic",
"version_major": "dynamic"
},
"processor": {
"event": "metric",
"name": "metric"
},
"service": {
"name": "systemtest",
"node": {
"name": "beowulf"
}
},
"timeseries": {
"instance": "systemtest:name:875650e21029d5b"
},
"transaction": {
"duration": {
"histogram": {
"counts": [
1
],
"values": [
1003519
]
}
},
"name": "name",
"root": true,
"type": "page-load"
},
"user_agent": {
"name": "Chrome"
}
}
]
}
39 changes: 2 additions & 37 deletions x-pack/apm-server/aggregation/txmetrics/aggregator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ type Aggregator struct {
config AggregatorConfig
metrics aggregatorMetrics
tooManyGroupsLogger *logp.Logger
userAgentLookup *userAgentLookup

mu sync.RWMutex
active, inactive *metrics
Expand Down Expand Up @@ -88,10 +87,6 @@ type AggregatorConfig struct {
// to maintain in the HDR Histograms. HDRHistogramSignificantFigures
// must be in the range [1,5].
HDRHistogramSignificantFigures int

// RUMUserAgentLRUSize is the size of the LRU cache for mapping RUM
// page-load User-Agent strings to browser names.
RUMUserAgentLRUSize int
}

// Validate validates the aggregator config.
Expand All @@ -108,9 +103,6 @@ func (config AggregatorConfig) Validate() error {
if n := config.HDRHistogramSignificantFigures; n < 1 || n > 5 {
return errors.Errorf("HDRHistogramSignificantFigures (%d) outside range [1,5]", n)
}
if config.RUMUserAgentLRUSize <= 0 {
return errors.New("RUMUserAgentLRUSize unspecified or negative")
}
return nil
}

Expand All @@ -122,16 +114,11 @@ func NewAggregator(config AggregatorConfig) (*Aggregator, error) {
if config.Logger == nil {
config.Logger = logp.NewLogger(logs.TransactionMetrics)
}
ual, err := newUserAgentLookup(config.RUMUserAgentLRUSize)
if err != nil {
return nil, err
}
return &Aggregator{
stopping: make(chan struct{}),
stopped: make(chan struct{}),
config: config,
tooManyGroupsLogger: config.Logger.WithOptions(logs.WithRateLimit(tooManyGroupsLoggerRateLimit)),
userAgentLookup: ual,
active: newMetrics(config.MaxTransactionGroups),
inactive: newMetrics(config.MaxTransactionGroups),
}, nil
Expand Down Expand Up @@ -356,15 +343,6 @@ func (a *Aggregator) updateTransactionMetrics(key transactionAggregationKey, has
}

func (a *Aggregator) makeTransactionAggregationKey(tx *model.Transaction) transactionAggregationKey {
var userAgentName string
if tx.Type == "page-load" {
// The APM app in Kibana has a special case for "page-load"
// transaction types, rendering distributions by country and
// browser. We use the same logic to decide whether or not
// to include user_agent.name in the aggregation key.
userAgentName = a.userAgentLookup.getUserAgentName(tx.Metadata.UserAgent.Original)
}

return transactionAggregationKey{
traceRoot: tx.ParentID == "",
transactionName: tx.Name,
Expand All @@ -380,10 +358,6 @@ func (a *Aggregator) makeTransactionAggregationKey(tx *model.Transaction) transa
hostname: tx.Metadata.System.Hostname(),
containerID: tx.Metadata.System.Container.ID,
kubernetesPodName: tx.Metadata.System.Kubernetes.PodName,

userAgentName: userAgentName,

// TODO(axw) clientCountryISOCode, requires geoIP lookup in apm-server.
}
}

Expand All @@ -403,10 +377,6 @@ func makeMetricset(key transactionAggregationKey, hash uint64, ts time.Time, cou
Container: model.Container{ID: key.containerID},
Kubernetes: model.Kubernetes{PodName: key.kubernetesPodName},
},
UserAgent: model.UserAgent{
Name: key.userAgentName,
},
// TODO(axw) include client.geo.country_iso_code somewhere
},
Event: model.MetricsetEventCategorization{
Outcome: key.transactionOutcome,
Expand Down Expand Up @@ -456,10 +426,8 @@ type metricsMapEntry struct {
}

type transactionAggregationKey struct {
traceRoot bool
agentName string
// TODO(axw) requires geoIP lookup in apm-server.
//clientCountryISOCode string
traceRoot bool
agentName string
containerID string
hostname string
kubernetesPodName string
Expand All @@ -470,7 +438,6 @@ type transactionAggregationKey struct {
transactionOutcome string
transactionResult string
transactionType string
userAgentName string
}

func (k *transactionAggregationKey) hash() uint64 {
Expand All @@ -479,7 +446,6 @@ func (k *transactionAggregationKey) hash() uint64 {
h.WriteString("1")
}
h.WriteString(k.agentName)
// TODO(axw) clientCountryISOCode
h.WriteString(k.containerID)
h.WriteString(k.hostname)
h.WriteString(k.kubernetesPodName)
Expand All @@ -490,7 +456,6 @@ func (k *transactionAggregationKey) hash() uint64 {
h.WriteString(k.transactionOutcome)
h.WriteString(k.transactionResult)
h.WriteString(k.transactionType)
h.WriteString(k.userAgentName)
return h.Sum64()
}

Expand Down
Loading

0 comments on commit 85b27f9

Please sign in to comment.