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

Add config for the the size of in-memory queue used for log/console data #93

Closed
iapaddler opened this issue Feb 3, 2021 · 3 comments · May be fixed by #689
Closed

Add config for the the size of in-memory queue used for log/console data #93

iapaddler opened this issue Feb 3, 2021 · 3 comments · May be fixed by #689
Assignees
Labels

Comments

@iapaddler
Copy link
Contributor

Should be user configurable

@iapaddler iapaddler added this to the 0.6 milestone Feb 3, 2021
@coccyx coccyx added this to To do in 0.6 Release Feb 4, 2021
@iapaddler iapaddler removed this from the 0.6 milestone Feb 8, 2021
@jrcheli jrcheli removed this from To do in 0.6 Release Feb 10, 2021
@iapaddler iapaddler added this to the Backlog milestone Oct 27, 2021
@ledbit ledbit modified the milestones: Backlog, Next Major (1.0.0) Nov 29, 2021
@jrcheli jrcheli assigned michalbiesek and unassigned iapaddler Dec 16, 2021
michalbiesek added a commit to michalbiesek/appscope that referenced this issue Dec 17, 2021
- handle flush bytes limit via environment variable
`SCOPE_LOG_FLUSH_LIMIT` and config parameter
- keep old value as default 32768

closes criblio#93
@michalbiesek
Copy link
Contributor

Possible duplicate for #189

michalbiesek added a commit to michalbiesek/appscope that referenced this issue Dec 20, 2021
- handle flush bytes limit via environment variable
`SCOPE_LOG_BUFFER_THRESHOLD` and config parameter
- keep old value as default 32768

closes criblio#93
michalbiesek added a commit to michalbiesek/appscope that referenced this issue Dec 20, 2021
- handle flush bytes limit via environment variable
`SCOPE_LOG_BUFFER_THRESHOLD` and config parameter
- keep old value as default 32768

closes criblio#93
@iapaddler
Copy link
Contributor Author

Closing this and replacing with issue #690. The salient concern is overall memory size and not just console data.

@iapaddler iapaddler removed this from the Next Major (1.0.0) milestone Dec 20, 2021
@jrcheli
Copy link
Contributor

jrcheli commented Jan 19, 2022

My review comments:

Naming/Configuration structure

The current state of this issue defines these two env vars:
SCOPE_LOG_BUFFER_THRESHOLD
SCOPE_LOG_FLUSH_PERIOD
with this structure:

        "libscope": {
          "log": {
            "bufthreshold": 32768,
            "flushperiod": 2000,
            "level": "warning",
            "transport": {
              "type": "file",
              "path": "/tmp/scope.log",
              "buffering": "line"
            }
          },
          "configevent": "true",
          "summaryperiod": 10,
          "commanddir": "/tmp"
        },

What I'd propose is renaming the env vars to:
SCOPE_CONSOLE_BUFFER_THRESHOLD
SCOPE_CONSOLE_FLUSH_PERIOD
with this structure:

        "libscope": {
          "log": {
            "level": "warning",
            "transport": {
              "type": "file",
              "path": "/tmp/scope.log",
              "buffering": "line"
            }
          },
          "console": {
            "bufthreshold": 32768,
            "flushperiod": 2000,
          },
          "configevent": "true",
          "summaryperiod": 10,
          "commanddir": "/tmp"
        },

Comments in scope.yml

We'll need to be clear that these affect two different things:
the handling of 1) file and 2) console watch types

Testing

As part of testing these changes, we'll want to be sure to test "reconfiguration" of a process at least manually for these two new configuration variables. I think things look like they should work, but I wonder if we might be missing a call to ctlDisconnect(g_prevctl, CFG_LS); at the end of doConfig().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants