Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

monitoring: Also show aggregate metrics if split via By#23237

Merged
efritz merged 8 commits intomainfrom
ef/aggregate-monitoring
Jul 27, 2021
Merged

monitoring: Also show aggregate metrics if split via By#23237
efritz merged 8 commits intomainfrom
ef/aggregate-monitoring

Conversation

@efritz
Copy link
Copy Markdown
Contributor

@efritz efritz commented Jul 26, 2021

This updates the Observation.NewGroup constructor to add an additional aggregate row that excludes the By filter passed by the user when supplied. This allows engineers to quickly see the rate of individual types of requests if separated by a label.

Screen Shot 2021-07-26 at 11 23 28 AM

This also allows separate alerting on individual (any gitserver request) and aggregate requests (all gitserver requests).

@efritz efritz requested review from a team and bobheadxi July 26, 2021 16:32
@efritz efritz self-assigned this Jul 26, 2021
@sourcegraph-bot
Copy link
Copy Markdown
Contributor

sourcegraph-bot commented Jul 26, 2021

Notifying subscribers in CODENOTIFY files for diff 9c1dd35...6d73d70.

Notify File(s)
@bobheadxi monitoring/definitions/shared/codeintel.go
monitoring/definitions/shared/observation.go
monitoring/definitions/shared/queues.go
monitoring/definitions/shared/shared.go
monitoring/definitions/shared/workerutil.go
monitoring/definitions/shared/workerutil_resetter.go
@christinaforney doc/admin/observability/dashboards.md
@slimsag monitoring/definitions/shared/codeintel.go
monitoring/definitions/shared/observation.go
monitoring/definitions/shared/queues.go
monitoring/definitions/shared/shared.go
monitoring/definitions/shared/workerutil.go
monitoring/definitions/shared/workerutil_resetter.go
@sourcegraph/distribution doc/admin/observability/dashboards.md
monitoring/definitions/shared/codeintel.go
monitoring/definitions/shared/observation.go
monitoring/definitions/shared/queues.go
monitoring/definitions/shared/shared.go
monitoring/definitions/shared/workerutil.go
monitoring/definitions/shared/workerutil_resetter.go

Copy link
Copy Markdown
Member

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

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

nice! with some thoughts 🤔 not blockers tho IMO (except for the enforcing By comment maybe, but that might just be too hard)

Comment thread monitoring/definitions/shared/codeintel.go Outdated
Comment thread monitoring/definitions/shared/observation.go
@bobheadxi
Copy link
Copy Markdown
Member

Also, I'm rethinking the cosmetic appeal of [foobar] in the Group names:
image

Could we uppercase that and remove the brackets at some point, e.g.

Codintel observable: foobar
Workerutil observable: foobar

or even better, drop the Observable (that's more internal terminology than something to flex to customers):

Codintel: foobar
Workerutil: foobar

@efritz
Copy link
Copy Markdown
Contributor Author

efritz commented Jul 27, 2021

@bobheadxi Updated if you want to take another look. I'll merge but if you have other suggestions I'll handle them in a follow-up PR.

Comment thread monitoring/definitions/shared/observation.go Outdated
Comment thread monitoring/definitions/shared/observation.go Outdated
@efritz efritz enabled auto-merge (squash) July 27, 2021 20:15
@efritz efritz merged commit 3266f37 into main Jul 27, 2021
@efritz efritz deleted the ef/aggregate-monitoring branch July 27, 2021 20:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants