Skip to content
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

fix 100% cpu #13

Merged
merged 2 commits into from
Jan 23, 2023
Merged

fix 100% cpu #13

merged 2 commits into from
Jan 23, 2023

Conversation

DimaGolomozy
Copy link
Contributor

@DimaGolomozy DimaGolomozy commented Jan 9, 2023

well, this is quite funny... in go

var num float64 = 0.5
time.Duration(num) == 0. (because type Duration = int64)

this made time.Sleep(time.Duration(NextSendInterval) * time.Second) to be the same as time.Sleep(0 * time.Second)
that made the 100% CPU (as described here: #6) because the for loop never sleep

better to use an explicit time.Duration type. plus i added an option to override this property when creating the logger

i belive this fixes issues:
#11
#6

@DimaGolomozy DimaGolomozy changed the title fix-100p-cpu fix-100%-cpu Jan 9, 2023
@DimaGolomozy DimaGolomozy changed the title fix-100%-cpu fix 100% cpu Jan 9, 2023
royfur
royfur previously approved these changes Jan 19, 2023
@daidokoro
Copy link
Contributor

daidokoro commented Jan 22, 2023

@DimaGolomozy, it appears your commits are unsigned, this is a merge requirement on this repo.

You should be able to retroactively sign these commits then we'll re-approve and get this change merged.

@DimaGolomozy
Copy link
Contributor Author

@DimaGolomozy, it appears your commits are unsigned, this is a merge requirement on this repo.

You should be able to retroactively sign these commits then we'll re-approve and get this change merged.

both commits are signed now

daidokoro
daidokoro previously approved these changes Jan 22, 2023
@DimaGolomozy
Copy link
Contributor Author

it looks like I'm having problems with signing..
i guess you can force merge it

@DimaGolomozy
Copy link
Contributor Author

@daidokoro are you guys going to merge this?

@DimaGolomozy
Copy link
Contributor Author

@daidokoro also here.. commits are signed and verified

daidokoro
daidokoro previously approved these changes Jan 23, 2023
@daidokoro
Copy link
Contributor

@daidokoro also here.. commits are signed and verified

Excellent @DimaGolomozy , the verification works, but there are some conflicts for this one. Can you please resolve? Thanks in advance.

Signed-off-by: dimagolomozy <[email protected]>
Signed-off-by: dimagolomozy <[email protected]>
@DimaGolomozy
Copy link
Contributor Author

@daidokoro done

Copy link
Contributor

@daidokoro daidokoro left a comment

Choose a reason for hiding this comment

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

lgtm

@daidokoro daidokoro merged commit 8db9885 into coralogix:master Jan 23, 2023
@DimaGolomozy
Copy link
Contributor Author

lgtm

thanks.
so when a new version will be released?

@daidokoro
Copy link
Contributor

@DimaGolomozy, done

@daidokoro daidokoro mentioned this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants