-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
Changes to collect metrics #18
base: main
Are you sure you want to change the base?
Conversation
a0543c4
to
58ccebb
Compare
@eranra can you please review this? |
@@ -30,7 +30,7 @@ select_stress_profile() { | |||
heavy_containers_msg_per_sec=1000 | |||
low_containers_msg_per_sec=10 | |||
number_of_log_lines_between_reports=10; | |||
maximum_logfile_size=10485760; | |||
maximum_logfile_size=52428800; # default file size |
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.
@ajaygupta978 it looks like what you are trying to do is "not set" the log file rotation for your tests ... lete me suggest a better approach. I suggest revoking this change to maximum_logfile_size and instead adding a flag somewhere here https://github.com/ViaQ/cluster-logging-collector-benchmarks/blob/main/deploy_to_openshift.sh#L7 that allows controlling whether maximum_logfile_size
is set by the benchmark or not. Actually, to make things even easier you can default to that flag to false, so it by default it will not change the maximum_logfile_size
from default OCP value.
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.
@eranra Yes you are right. Thanks for suggestion now I think I have better suggestion. Instead of flag we can add max_log_size
variable as command line argument which can be set by user based on selected profile. Currently to change value I need to do change in source code. By default it will take OCP value.
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.
@ajaygupta978 agree this can force the value ... I am just suggesting that for example if someone put a zero in that value we will not even call the function to set rate limits and use what ever comes out of the box from OCP.
echo -e "\nOutput log file is: $OUTPUT_FILE\n"; | ||
touch $OUTPUT_FILE; |
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.
Did you check with all collectors that there is no need to touch $OUTPUT_FILE;
... I added that for some reason ... I actually don't remember why. What happens if this is kept ???
@@ -157,4 +157,4 @@ objects: | |||
maxUnavailable: 1 | |||
type: RollingUpdate | |||
parameters: | |||
- name: image_name |
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.
ok
inputs = ["ocp_sys.infra","ocp_sys.app"] | ||
encoding.codec = "ndjson" | ||
path = "/var/log/containers/stress.log" |
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.
ok
@@ -1,22 +1,40 @@ | |||
# This script collects metrics like log-per-sec, cpu-percentage, cpu-cores and memory required per iterations in running reg-ex rules | |||
# in discovering log-levels. | |||
|
|||
# collector values could be fluentd, fluentbit | |||
export collector=$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.
looks good to me. Maybe @jcantrill will want to take a quick second look at the changes to this file
@ajaygupta978: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR address changed done to collect metrics.