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

authz: expose metrics of authzFilter duration #9264

Merged
merged 3 commits into from
Mar 24, 2020
Merged

Conversation

unknwon
Copy link
Member

@unknwon unknwon commented Mar 24, 2020

This PR adds a p95 panel for authzFilter duration:
image

Fixes #8612

@unknwon unknwon requested a review from beyang as a code owner March 24, 2020 07:01
@unknwon unknwon requested review from a team March 24, 2020 07:01
@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #9264 into master will decrease coverage by 0.09%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #9264      +/-   ##
==========================================
- Coverage   41.46%   41.36%   -0.10%     
==========================================
  Files        1333     1324       -9     
  Lines       72296    71858     -438     
  Branches     6657     6603      -54     
==========================================
- Hits        29974    29725     -249     
+ Misses      39580    39391     -189     
  Partials     2742     2742              
Impacted Files Coverage Δ
enterprise/cmd/repo-updater/authz/perms_syncer.go 0.00% <0.00%> (ø)
cmd/frontend/db/repos_perm.go 75.40% <100.00%> (+0.40%) ⬆️
lsif/src/bundle-manager/backend/location.ts
lsif/src/bundle-manager/backend/database.ts
lsif/src/bundle-manager/tasks.ts
lsif/src/bundle-manager/settings.ts
lsif/src/bundle-manager/metrics.ts
lsif/src/bundle-manager/routes/uploads.ts
lsif/src/bundle-manager/manager.ts
lsif/src/bundle-manager/routes/database.ts
... and 1 more

tr, ctx := trace.New(ctx, "authzFilter", "")
defer func() {
if err != nil {
took := time.Now().Sub(began).Seconds()
defer tr.Finish()
Copy link
Member

Choose a reason for hiding this comment

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

why is this a defer now?

Copy link
Member Author

@unknwon unknwon Mar 24, 2020

Choose a reason for hiding this comment

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

I just used the same pattern I did for tr.Finish() in another place. Seems working fine, is there a reason we don't want to use defer tr.Finish()?

Copy link
Member

Choose a reason for hiding this comment

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

seems like an unnecessary defer since there is no early termination. However, it is correct. Was wondering if there was some other reason you changed the code I didn't spot, but lgtm

cmd/frontend/db/repos_perm.go Outdated Show resolved Hide resolved
cmd/frontend/db/repos_perm.go Outdated Show resolved Hide resolved
enterprise/cmd/repo-updater/authz/perms_syncer.go Outdated Show resolved Hide resolved
@unknwon unknwon merged commit 3591711 into master Mar 24, 2020
@unknwon unknwon deleted the jc/authz-filter-metrics branch March 24, 2020 14:03
unknwon added a commit that referenced this pull request Mar 24, 2020
* Expose metrics of authzFilter duration

* Use time.Since to calculate time took

* Update CHANGELOG
# Conflicts:
#	dev/Procfile
@unknwon unknwon mentioned this pull request Mar 25, 2020
16 tasks
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.

RFC 113: Instrumenting PermsSyncer
2 participants