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

Rename indexer metrics related to get operations #853

Merged
merged 8 commits into from
Jul 27, 2022
Merged

Rename indexer metrics related to get operations #853

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
02fb0ad
Rename indexer metrics related to get operations
mrodm Jul 26, 2022
6d32e1a
Update Changelog
mrodm Jul 26, 2022
da35579
Merge upstream main branch into update_storage_metrics branch
mrodm Jul 27, 2022
b56f239
Fix opts filter check
mrodm Jul 27, 2022
85fae6d
Moved to a const label string in storage indexer
mrodm Jul 27, 2022
7d64c94
Update changelog
mrodm Jul 27, 2022
8e810cb
Update help messages
mrodm Jul 27, 2022
7d15558
Replace hardcoded strings by const variables
mrodm Jul 27, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

* Return only the latest version of each package when a combined index is used. [#849](https://github.com/elastic/package-registry/pull/849)
* Return only first appearance of the same version of a package when it is available in multiple indexes. [#849](https://github.com/elastic/package-registry/pull/849)
* Rename indexer metrics related to get operations and add the indexer name label to it. [#853](https://github.com/elastic/package-registry/pull/853)

### Added

Expand Down
6 changes: 1 addition & 5 deletions indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@ package main
import (
"context"
"sort"
"time"

"github.com/Masterminds/semver/v3"

"github.com/elastic/package-registry/metrics"
"github.com/elastic/package-registry/packages"
)

Expand All @@ -37,8 +35,6 @@ func (c CombinedIndexer) Init(ctx context.Context) error {
}

func (c CombinedIndexer) Get(ctx context.Context, opts *packages.GetOptions) (packages.Packages, error) {
start := time.Now()
defer metrics.StorageIndexerGetDurationSeconds.Observe(time.Since(start).Seconds())
var packages packages.Packages
for _, indexer := range c {
p, err := indexer.Get(ctx, opts)
Expand All @@ -48,7 +44,7 @@ func (c CombinedIndexer) Get(ctx context.Context, opts *packages.GetOptions) (pa
packages = packages.Join(p)
}

if !opts.Filter.AllVersions {
if opts != nil && opts.Filter != nil && !opts.Filter.AllVersions {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was needed after updating this branch with main.
This Get method is called with opts as nil from https://github.com/elastic/package-registry/blob/main/main.go#L290

return latestPackagesVersion(packages), nil
}

Expand Down
2 changes: 1 addition & 1 deletion main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ var (
func init() {
flag.BoolVar(&printVersionInfo, "version", false, "Print Elastic Package Registry version")
flag.StringVar(&address, "address", "localhost:8080", "Address of the package-registry service.")
flag.StringVar(&metricsAddress, "metrics-address", "", "Address to expose the Prometheus metrics.")
flag.StringVar(&metricsAddress, "metrics-address", "", "Address to expose the Prometheus metrics (experimental). ")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a minor change here to add this Prometheus metric endpoint is subject to changes.
Do you think it is worthy to add it ?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me, we may redefine something else while we start using it.

flag.StringVar(&tlsCertFile, "tls-cert", "", "Path of the TLS certificate.")
flag.StringVar(&tlsKeyFile, "tls-key", "", "Path of the TLS key.")
flag.StringVar(&configPath, "config", "config.yml", "Path to the configuration file.")
Expand Down
11 changes: 6 additions & 5 deletions metrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,32 +39,33 @@ var (
prometheus.CounterOpts{
Namespace: metricsNamespace,
Name: "storage_indexer_update_index_success_total",
Help: "A counter for updates of the cursor.",
Help: "A counter for updates of the cursor in the storage indexer.",
},
)

StorageIndexerUpdateIndexErrorsTotal = prometheus.NewCounter(
prometheus.CounterOpts{
Namespace: metricsNamespace,
Name: "storage_indexer_update_index_error_total",
Help: "A counter for all the update index processes that finished with error.",
Help: "A counter for all the update index processes that finished with error in the storage indexer.",
},
)

StorageIndexerUpdateIndexDurationSeconds = prometheus.NewHistogram(
prometheus.HistogramOpts{
Namespace: metricsNamespace,
Name: "storage_indexer_update_index_duration_seconds",
Help: "A histogram of latencies for update index processes run by the indexer.",
Help: "A histogram of latencies for update index processes run by the storage indexer.",
},
)

StorageIndexerGetDurationSeconds = prometheus.NewHistogram(
IndexerGetDurationSeconds = prometheus.NewHistogramVec(
Copy link
Member

Choose a reason for hiding this comment

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

Umm, I am wondering how useful is this metric. Looking at the sample metrics in the description, they seem to be all quite fast as these operations happen mostly in memory, and they end up all in the lower bucket.
We also have metrics in the middleware for /search, that may be more useful for final users, as it includes the time of the whole response.

So I wonder if maybe we can remove this metric. Or if we leave it, lower the boundaries of each bucket so they don't end up all in the lower bucket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently I don't have data about these get operations in the storage indexer.
I was expecting that this metric would be more significant in that storage indexer than in the file system indexers. I was thinking that maybe those get ops against storage indexer would be slower (just hypothesis, I don't have data about this).

Copy link
Member

Choose a reason for hiding this comment

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

Well, current implementations are only filtering a list in memory. But it is true that the operation in the storage indexer is in a mutex whose lock can have some wait time while the index is updated. We may revisit the bucket thresholds when we have more data.

prometheus.HistogramOpts{
Namespace: metricsNamespace,
Name: "storage_indexer_get_duration_seconds",
Name: "indexer_get_duration_seconds",
Help: "A histogram of latencies for get processes run by the indexer.",
},
[]string{"indexer"},
)
)

Expand Down
2 changes: 1 addition & 1 deletion metrics/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func MetricsMiddleware() mux.MiddlewareFunc {

prometheus.MustRegister(NumberIndexedPackages)
prometheus.MustRegister(StorageRequestsTotal)
prometheus.MustRegister(StorageIndexerGetDurationSeconds)
prometheus.MustRegister(IndexerGetDurationSeconds)
prometheus.MustRegister(StorageIndexerUpdateIndexDurationSeconds)
prometheus.MustRegister(StorageIndexerUpdateIndexSuccessTotal)
prometheus.MustRegister(StorageIndexerUpdateIndexErrorsTotal)
Expand Down
5 changes: 5 additions & 0 deletions packages/packages.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@ import (
"os"
"path/filepath"
"strings"
"time"

"github.com/Masterminds/semver/v3"
"github.com/pkg/errors"
"github.com/prometheus/client_golang/prometheus"
"go.elastic.co/apm"
"go.uber.org/zap"

"github.com/elastic/package-registry/metrics"
"github.com/elastic/package-registry/util"
)

Expand Down Expand Up @@ -192,6 +195,8 @@ func (i *FileSystemIndexer) Init(ctx context.Context) (err error) {
// This assumes changes to packages only happen on restart (unless development mode is enabled).
// Caching the packages request many file reads every time this method is called.
func (i *FileSystemIndexer) Get(ctx context.Context, opts *GetOptions) (Packages, error) {
start := time.Now()
defer metrics.IndexerGetDurationSeconds.With(prometheus.Labels{"indexer": i.label}).Observe(time.Since(start).Seconds())
if opts == nil {
return i.packageList, nil
}
Expand Down
6 changes: 6 additions & 0 deletions storage/indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,16 @@ import (

"cloud.google.com/go/storage"
"github.com/pkg/errors"
"github.com/prometheus/client_golang/prometheus"
"go.uber.org/zap"

"github.com/elastic/package-registry/metrics"
"github.com/elastic/package-registry/packages"
"github.com/elastic/package-registry/util"
)

const indexerGetDurationPrometheusLabel = "StorageIndexer"

type Indexer struct {
options IndexerOptions
storageClient *storage.Client
Expand Down Expand Up @@ -173,6 +176,9 @@ func (i *Indexer) updateIndex(ctx context.Context) error {
}

func (i *Indexer) Get(ctx context.Context, opts *packages.GetOptions) (packages.Packages, error) {
start := time.Now()
defer metrics.IndexerGetDurationSeconds.With(prometheus.Labels{"indexer": indexerGetDurationPrometheusLabel}).Observe(time.Since(start).Seconds())

i.m.RLock()
defer i.m.RUnlock()

Expand Down