Skip to content

feat: extended metrics #66

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
Merged
merged 15 commits into from
Nov 7, 2019
Merged

feat: extended metrics #66

merged 15 commits into from
Nov 7, 2019

Conversation

pawels-optimizely
Copy link
Contributor

No description provided.

@pawels-optimizely
Merge branch 'master' into pawel/OASIS-5346
2b9c395 
# Conflicts:
#	pkg/api/middleware/metrics.go
#	pkg/api/middleware/metrics_test.go
#	pkg/api/router.go
@pawels-optimizely
some linter fixes
1f266f3
@pawels-optimizely
adjusting tests and linters
a778202
@codecov
Copy link

codecov bot commented Nov 6, 2019 •
edited
Loading

Codecov Report

Merging #66 into master will increase coverage by 0.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
+ Coverage   74.16%   74.24%   +0.08%     
==========================================
  Files          19       19              
  Lines         507      528      +21     
==========================================
+ Hits          376      392      +16     
- Misses        109      114       +5     
  Partials       22       22
Impacted Files Coverage Δ
pkg/api/router.go 70.83% <100%> (+1.26%) ⬆️
pkg/api/middleware/metrics.go 82.14% <78.26%> (-17.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4afa376...be3cbf5. Read the comment docs.

mikecdavis
mikecdavis suggested changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@mikecdavis mikecdavis left a comment

Choose a reason for hiding this comment

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

It's looking good, but I think we can simplify things a bit by removing the MetricsCollection and explicitly naming the metrics as opposed to computing them from the request.

mikecdavis
mikecdavis approved these changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@mikecdavis mikecdavis left a comment

Choose a reason for hiding this comment

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

Approved!! Looks good, but maybe change the name from "UpdateRouteMetrics(...)" to "Metrics(...)" or something.

@pawels-optimizely pawels-optimizely merged commit 8fbdd90 into master Nov 7, 2019
@pawels-optimizely pawels-optimizely deleted the pawel/OASIS-5346 branch November 7, 2019 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeproeng37 mikeproeng37 Awaiting requested review from mikeproeng37

1 more reviewer

@mikecdavis mikecdavis mikecdavis approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pawels-optimizely @mikecdavis