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(profiler): do not collect disabled profile types #2836

Merged
merged 10 commits into from Sep 12, 2020
Merged

fix(profiler): do not collect disabled profile types #2836

merged 10 commits into from Sep 12, 2020

Conversation

@kalyanac
Copy link
Contributor

@kalyanac kalyanac commented Sep 10, 2020

Fixes #2835

@kalyanac kalyanac requested review from codyoss and tbpg as code owners Sep 10, 2020
@google-cla google-cla bot added the cla: yes label Sep 10, 2020
@kalyanac kalyanac requested a review from aalexand Sep 10, 2020
@tbpg
tbpg approved these changes Sep 10, 2020
@kalyanac kalyanac requested a review from jqll Sep 10, 2020
@jqll
jqll approved these changes Sep 10, 2020
@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

heapMu

@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

OOMProfiling (similar to NoHeapProfiling and friends above)

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand replied Sep 12, 2020

Is this variable used? I couldn't find its use.

@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

The units are unclear - please add them to the name of the constant. E.g OOMProfileThresholdKiBs or alike.

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand replied Sep 12, 2020

Also unclear why it is float32 - an integer would be probably suffice.

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand replied Sep 12, 2020

If this is ratio then document so and name as OOMProfileRatioThreshold

@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

thresholds -> threshold?

@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

Using https://golang.org/pkg/io/ioutil/#ReadFile will simplify this code.

@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

Check memLimitBytes for zero to avoid division by zero which will crash the profiled program.

@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

Invert the condition and continue to unindent the code after it.

@aalexand

This comment has been minimized.

Copy link
Collaborator

@aalexand aalexand commented on profiler/profiler.go in d77d11a Sep 12, 2020

We should factor out the code that populates the deployment since it's the same for regular profiles.

@kalyanac kalyanac marked this pull request as draft Sep 12, 2020
kalyanac added 3 commits Sep 12, 2020
This reverts commit 5746079.
This reverts commit d77d11a.
@kalyanac kalyanac marked this pull request as ready for review Sep 12, 2020
@kalyanac
Copy link
Contributor Author

@kalyanac kalyanac commented Sep 12, 2020

I pushed a commit by mistake, reverted it now. Please merge once tests pass.

@kalyanac kalyanac merged commit faeb498 into googleapis:master Sep 12, 2020
3 checks passed
3 checks passed
@google-cla
cla/google All necessary CLAs are signed
@conventional-commit-lint-gcf
conventionalcommits.org
Details
@kokoro-team
kokoro Kokoro CI build successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants