-
Notifications
You must be signed in to change notification settings - Fork 44
add hostname to the labels of the metrics; pull out calls to metrics … #707
base: master
Are you sure you want to change the base?
add hostname to the labels of the metrics; pull out calls to metrics … #707
Conversation
chmeliik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable 👍
Are there any tests for these metrics that could be updated? If we have none, that's probably OK
I don't think there are any :( WIth this change, it should be easy enough to add unit tests for metrics.py. I also agree it's probably OK to have this merged without the tests, they can be added as an improvement later. |
|
Overall, LGTM. Only fix needed is adding type annotations, as mentioned. |
31ec047 to
ec499c5
Compare
|
What's the status of this PR? Is it pending review or pending changes? |
c87f8ae to
b5256c8
Compare
…file Signed-off-by: Mike Kingsbury <[email protected]>
Signed-off-by: Mike Kingsbury <[email protected]>
b5256c8 to
bf16576
Compare
|
Should be now pending review, changes were made to the annotations |
| mako==1.2.2 \ | ||
| --hash=sha256:3724869b363ba630a272a5f89f68c070352137b8fd1757650017b7e06fda163f \ | ||
| --hash=sha256:8efcb8004681b5f71d09c983ad5a9e6f5c40601a6ec469148753292abc0da534 | ||
| mako==1.2.1 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the requirements*.txt changes intentional? A lot of dependencies got downgraded, as well as the python version used to generate the files
| @@ -1,5 +1,5 @@ | |||
| # | |||
| # This file is autogenerated by pip-compile with python 3.10 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we keep running pip-compile using python 3.10 since it's the newest version? (if there is any difference between using 3.9 and 3.10). Apart from that, LGTM
|
Let me revisit this, I may have done the wrong thing during a rebase...
…On Tue, Nov 22, 2022 at 4:25 AM Adam Cmiel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In requirements-web.txt
<#707 (comment)>
:
> -mako==1.2.2 \
- --hash=sha256:3724869b363ba630a272a5f89f68c070352137b8fd1757650017b7e06fda163f \
- --hash=sha256:8efcb8004681b5f71d09c983ad5a9e6f5c40601a6ec469148753292abc0da534
+mako==1.2.1 \
Are the requirements*.txt changes intentional? A lot of dependencies got
downgraded, as well as the python version used to generate the files
—
Reply to this email directly, view it on GitHub
<#707 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ATFBKRMKLNZYZDUQUUPJQUTWJSGRDANCNFSM6AAAAAAQMXQFVE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Adds hostnames to metrics; pull out metrics functionality to metrics.py file, suggested by Bruno.
Signed-off-by: Mike Kingsbury [email protected]
Maintainers will complete the following section