chore: default deployment with collect_metric=true and DEBUG level #1148
Conversation
DvirYo-starkware
requested review from
dan-starkware and
eranreshef-starkware
Codecov Report
@@ Coverage Diff @@
## main #1148 +/- ##
=======================================
Coverage 74.08% 74.08%
=======================================
Files 74 74
Lines 6636 6636
Branches 6636 6636
=======================================
Hits 4916 4916
Misses 873 873
Partials 847 847 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
deployments/helm/values.yaml
Outdated
| # If true, metrics will be collected and exposed on the monitoring port. | ||
| collect_metrics: true |
There was a problem hiding this comment.
- An application that runs inside a cluster (papyrus in this case) can't guarantee that its metrics would be collected by some other app.
- I don't see anywhere in the helm chart which reference this condition.
| env: | ||
| - name: RUST_LOG | ||
| value: papyrus=DEBUG |
There was a problem hiding this comment.
It would be easier to place this key-value pair in the configmap which it's content is exported as env-vars
DvirYo-starkware
changed the title
deployments/helm/values.yaml
Outdated
| # If true, metrics will be collected and exposed on the monitoring port. | ||
| # Note: this is not a guarantee the metrics will be collected. To see where | ||
| # they collected see the deployment.yaml file. |
There was a problem hiding this comment.
The original phrasing was fine, just remove the collected and part.
deployments/helm/values.yaml
Outdated
| @@ -1,7 +1,7 @@ | |||
| # Default values for a papyrus deployment. | |||
|
|
|||
| # The verbosity level of logs ("debug", "info", "error", etc.) | |||
| rustLogLevel: "info" | |||
| rustLogLevel: "debug" | |||
There was a problem hiding this comment.
default log level should be "info"
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @eranreshef-starkware)
deployments/helm/values.yaml line 14 at r1 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
- An application that runs inside a cluster (papyrus in this case) can't guarantee that its metrics would be collected by some other app.
- I don't see anywhere in the helm chart which reference this condition.
Done.
deployments/helm/values.yaml line 4 at r2 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
default log level should be "info"
Why? We want the default value in the deployment to be "debug".
deployments/helm/values.yaml line 15 at r2 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
The original phrasing was fine, just remove the
collected andpart.
Done.
deployments/helm/values.yaml line 16 at r2 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
I don't see where this value is being used
See the deployment file line 68.
deployments/helm/templates/deployment.yaml line 62 at r1 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
It would be easier to place this key-value pair in the configmap which it's content is exported as env-vars
I decided to use the already existing RUST_LOG value from the config map.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
deployments/helm/values.yaml line 4 at r2 (raw file):
Previously, DvirYo-starkware wrote…
Why? We want the default value in the deployment to be "debug".
who wants it and why?
deployments/helm/values.yaml line 15 at r2 (raw file):
Previously, DvirYo-starkware wrote…
Done.
you removed the wrong part. metrics are not being collected automatically.
deployments/helm/values.yaml line 16 at r2 (raw file):
Previously, DvirYo-starkware wrote…
See the deployment file line 68.
missed it, sorry.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @dan-starkware and @eranreshef-starkware)
deployments/helm/values.yaml line 4 at r2 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
who wants it and why?
I changed it back to info. This is not so important.
deployments/helm/values.yaml line 15 at r2 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
you removed the wrong part. metrics are not being collected automatically.
There is a need to collect the metrics in the node itself; without this collection, the node will not have the statics to return.
I rephrased the comment to emphasize this difference.
|
Previously, DvirYo-starkware wrote…
The helm chart and it's documentation shouldn't expose any information regarding the internal implementation. even if papyrus is open source and everyone can see its internal logic, the helm chart "doesn't care" about it. here you just need to explain that this boolean value is enabling or disabling the metrics endpoint. |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dan-starkware and @eranreshef-starkware)
deployments/helm/values.yaml line 15 at r2 (raw file):
Previously, eranreshef-starkware (Eran Reshef) wrote…
The helm chart and it's documentation shouldn't expose any information regarding the internal implementation. even if papyrus is open source and everyone can see its internal logic, the helm chart "doesn't care" about it. here you just need to explain that this boolean value is enabling or disabling the metrics endpoint.
Just write here precisely what you think should be here.
|
Previously, DvirYo-starkware wrote…
if it was me implementing this feature, I would have added a new section to the values file for metrics related configuration. something like: This way we can later add more parameters to this domain (like the metrics_port and scrape annotations for example). |
|
There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. |
…level.
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information
This change is