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

Jump to bottom

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 2 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Bugfixes

* Rename indexer metrics related to get operations. [#853](https://github.com/elastic/package-registry/pull/853)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be moved to another section in the changelog ?

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 it can fit as a bugfix 🙂 Though it could be also seen as a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

A bugfix is fine :)

### Added

* Add `elastic.subscription` condition to package index metadata, use this value for backwards compatibility with previous `license` field. [#826](https://github.com/elastic/package-registry/pull/826)
Expand Down
4 changes: 0 additions & 4 deletions indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ package main

import (
"context"
"time"

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

Expand All @@ -34,8 +32,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 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 @@ -159,6 +162,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
7 changes: 7 additions & 0 deletions storage/indexer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ 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"
Expand All @@ -31,6 +32,8 @@ type Indexer struct {
m sync.RWMutex

resolver packages.RemoteResolver

label string
}

type IndexerOptions struct {
Expand All @@ -43,6 +46,7 @@ func NewIndexer(storageClient *storage.Client, options IndexerOptions) *Indexer
return &Indexer{
storageClient: storageClient,
options: options,
label: "StorageIndexer",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. could it be a const instead of a structure field?

Copy link
Contributor Author

@mrodm mrodm Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wanted to follow the same structure that it is used for FileSystemIndexers (link) where the label is part of the struct. But it is true that for storage indexer it is not needed to add that label as a field struct. So, I'll move it to be a const.

}
}

Expand Down Expand Up @@ -173,6 +177,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": i.label}).Observe(time.Since(start).Seconds())

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

Expand Down