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.

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

[Metricbeat/Kibana/stats] Enforce exclude_usage=true #22732

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,7 @@ same journal. {pull}18467[18467]
- Add connection and route metricsets for nats metricbeat module to collect metrics per connection/route. {pull}22445[22445]
- Add unit file states to system/service {pull}22557[22557]
- Add io.ops in fields exported by system.diskio. {pull}22066[22066]
- `kibana` module: `stats` metricset no-longer collects usage-related data. {pull}22732[22732]

*Packetbeat*

Expand Down
45 changes: 11 additions & 34 deletions metricbeat/module/kibana/stats/stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package stats

import (
"fmt"
"strconv"
"strings"
"time"

Expand All @@ -38,10 +37,8 @@ func init() {
}

const (
statsPath = "api/stats"
settingsPath = "api/settings"
usageCollectionPeriod = 24 * time.Hour
usageCollectionBackoff = 1 * time.Hour
statsPath = "api/stats"
settingsPath = "api/settings"
)

var (
Expand All @@ -55,11 +52,9 @@ var (
// MetricSet type defines all fields of the MetricSet
type MetricSet struct {
*kibana.MetricSet
statsHTTP *helper.HTTP
settingsHTTP *helper.HTTP
usageLastCollectedOn time.Time
usageNextCollectOn time.Time
isUsageExcludable bool
statsHTTP *helper.HTTP
settingsHTTP *helper.HTTP
isUsageExcludable bool
}

// New create a new instance of the MetricSet
Expand Down Expand Up @@ -157,31 +152,17 @@ func (m *MetricSet) fetchStats(r mb.ReporterV2, now time.Time) error {
var content []byte
var err error

// Collect usage stats only once every usageCollectionPeriod
// Add exclude_usage=true if the Kibana Version supports it
if m.isUsageExcludable {
origURI := m.statsHTTP.GetURI()
defer m.statsHTTP.SetURI(origURI)

shouldCollectUsage := m.shouldCollectUsage(now)
m.statsHTTP.SetURI(origURI + "&exclude_usage=" + strconv.FormatBool(!shouldCollectUsage))

content, err = m.statsHTTP.FetchContent()
if err != nil {
if shouldCollectUsage {
// When errored in collecting the usage stats it may be counterproductive to try again on the next poll, try to collect the stats again after usageCollectionBackoff
m.usageNextCollectOn = now.Add(usageCollectionBackoff)
}
return err
}
m.statsHTTP.SetURI(origURI + "&exclude_usage=true")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change we no longer need the comment above if m.isUsageExcludable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted! I updated it to explain what's actually happening now :)
Thank you!


if shouldCollectUsage {
m.usageLastCollectedOn = now
}
} else {
content, err = m.statsHTTP.FetchContent()
if err != nil {
return err
}
content, err = m.statsHTTP.FetchContent()
if err != nil {
return err
}

if m.XPackEnabled {
Expand Down Expand Up @@ -219,7 +200,3 @@ func (m *MetricSet) fetchSettings(r mb.ReporterV2, now time.Time) {
func (m *MetricSet) calculateIntervalMs() int64 {
return m.Module().Config().Period.Nanoseconds() / 1000 / 1000
}

func (m *MetricSet) shouldCollectUsage(now time.Time) bool {
return now.Sub(m.usageLastCollectedOn) > usageCollectionPeriod && now.Sub(m.usageNextCollectOn) > 0
}
65 changes: 24 additions & 41 deletions metricbeat/module/kibana/stats/stats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,14 @@ import (
"net/http"
"net/http/httptest"
"testing"
"time"

"github.com/stretchr/testify/require"

mbtest "github.com/elastic/beats/v7/metricbeat/mb/testing"
"github.com/elastic/beats/v7/metricbeat/module/kibana/mtest"
)

func TestFetchUsage(t *testing.T) {
func TestFetchExcludeUsage(t *testing.T) {
// Spin up mock Kibana server
numStatsRequests := 0
kib := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Expand All @@ -45,17 +44,15 @@ func TestFetchUsage(t *testing.T) {
// Make GET /api/stats return 503 for first call, 200 for subsequent calls
switch numStatsRequests {
case 0: // first call
require.Equal(t, "false", excludeUsage)
require.Equal(t, "true", excludeUsage) // exclude_usage is always true
w.WriteHeader(503)

case 1: // second call
// Make sure exclude_usage is true since first call failed and it should not try again until usageCollectionBackoff time has passed
require.Equal(t, "true", excludeUsage)
require.Equal(t, "true", excludeUsage) // exclude_usage is always true
w.WriteHeader(200)

case 2: // third call
// Make sure exclude_usage is still true
require.Equal(t, "true", excludeUsage)
require.Equal(t, "true", excludeUsage) // exclude_usage is always true
w.WriteHeader(200)
}

Expand All @@ -78,39 +75,25 @@ func TestFetchUsage(t *testing.T) {
mbtest.ReportingFetchV2Error(f)
}

func TestShouldCollectUsage(t *testing.T) {
now := time.Now()

cases := map[string]struct {
usageLastCollectedOn time.Time
usageNextCollectOn time.Time
expectedResult bool
}{
"within_usage_collection_period": {
usageLastCollectedOn: now.Add(-1 * usageCollectionPeriod),
expectedResult: false,
},
"after_usage_collection_period_but_before_next_scheduled_collection": {
usageLastCollectedOn: now.Add(-2 * usageCollectionPeriod),
usageNextCollectOn: now.Add(3 * time.Hour),
expectedResult: false,
},
"after_usage_collection_period_and_after_next_scheduled_collection": {
usageLastCollectedOn: now.Add(-2 * usageCollectionPeriod),
usageNextCollectOn: now.Add(-1 * time.Hour),
expectedResult: true,
},
}

for name, test := range cases {
t.Run(name, func(t *testing.T) {
m := MetricSet{
usageLastCollectedOn: test.usageLastCollectedOn,
usageNextCollectOn: test.usageNextCollectOn,
}
func TestFetchNoExcludeUsage(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding this test. 👍

// Spin up mock Kibana server
kib := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/api/status":
w.Write([]byte("{ \"version\": { \"number\": \"7.0.0\" }}")) // v7.0.0 does not support exclude_usage and should not be sent

actualResult := m.shouldCollectUsage(now)
require.Equal(t, test.expectedResult, actualResult)
})
}
case "/api/stats":
excludeUsage := r.FormValue("exclude_usage")
require.Empty(t, excludeUsage) // exclude_usage should not be provided
w.WriteHeader(200)
}
}))
defer kib.Close()

config := mtest.GetConfig("stats", kib.URL, true)

f := mbtest.NewReportingMetricSetV2Error(t, config)

// First fetch
mbtest.ReportingFetchV2Error(f)
}