Skip to content
This repository was archived by the owner on Dec 23, 2023. It is now read-only.

Disable by default RunningSpanStore, and enable if Zpages are initialized.#1875

Merged
bogdandrutu merged 3 commits intocensus-instrumentation:masterfrom
bogdandrutu:nooprss
May 4, 2019
Merged

Disable by default RunningSpanStore, and enable if Zpages are initialized.#1875
bogdandrutu merged 3 commits intocensus-instrumentation:masterfrom
bogdandrutu:nooprss

Conversation

@bogdandrutu
Copy link
Copy Markdown
Contributor

@bogdandrutu bogdandrutu commented May 3, 2019 •
edited
Loading

Fixes #1813

@bogdandrutu bogdandrutu requested review from a team, dinooliva, rghetia and songy23 as code owners May 3, 2019 22:12
songy23
songy23 approved these changes May 3, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

I'm good with going with this approach first. However if users have zPages enabled and experience the memory issue, I think ultimately we still need the non-blocking option in #1837.

Please also add this to CHANGELOG.

Comment thread api/src/main/java/io/opencensus/trace/export/RunningSpanStore.java
* Enables the {@code RunningSpanStore}. Default the {@code RunningSpanStore} is not enabled.
*
* @param enable {@code true} if the {@code RunningSpanStore} should be enabled.
*/
Copy link
Copy Markdown
Contributor

@songy23 songy23 May 3, 2019

Choose a reason for hiding this comment

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

Nit: missing a @since tag.

Copy link
Copy Markdown
Contributor Author

@bogdandrutu bogdandrutu May 3, 2019

Choose a reason for hiding this comment

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

Done.

rghetia
rghetia approved these changes May 3, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@rghetia rghetia left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread impl_core/src/main/java/io/opencensus/implcore/trace/export/InProcessRunningSpanStore.java Outdated
@@ -0,0 +1,155 @@
/*
* Copyright 2018, OpenCensus Authors
Copy link
Copy Markdown
Contributor

@rghetia rghetia May 3, 2019

Choose a reason for hiding this comment

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

nit: s/2018/2019/

Copy link
Copy Markdown
Contributor Author

@bogdandrutu bogdandrutu May 3, 2019

Choose a reason for hiding this comment

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

Done.

@bogdandrutu
Copy link
Copy Markdown
Contributor Author

bogdandrutu commented May 3, 2019

Updated changelog, also allow users to set a limit.

@bogdandrutu
Copy link
Copy Markdown
Contributor Author

bogdandrutu commented May 3, 2019

@songy23 this issue is totally independent of blocking. blocking can occur if users forget or not to close some of the spans. This issue is related to OOM-ing if Spans are not correctly ended.

@bogdandrutu bogdandrutu merged commit 68e19ed into census-instrumentation:master May 4, 2019
@bogdandrutu bogdandrutu deleted the nooprss branch May 4, 2019 00:09
@songy23 songy23 mentioned this pull request May 6, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dinooliva dinooliva Awaiting requested review from dinooliva

2 more reviewers

@rghetia rghetia rghetia approved these changes

@songy23 songy23 songy23 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Configure a max number spans in RunningSpanStoreImpl

4 participants

@bogdandrutu @rghetia @songy23 @googlebot