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.

Sign up for GitHub

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

Jump to bottom

Stats prepare for multiple queues per destdrv #2302

Merged
merged 13 commits into from
Sep 24, 2018
Merged

Stats prepare for multiple queues per destdrv #2302

merged 13 commits into from
Sep 24, 2018

Conversation

lbudai
Copy link
Collaborator

@lbudai lbudai commented Sep 21, 2018

No description provided.

@lbudai
logqueue: add method for add/sub memory usage counter
652f7dc 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
logqueue-fifo: use LogQueue API for modifying memory_usage counter
6a8116c 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
diskq: nonreliable: use LogQueue API for modifying memory_usage counter
7792380 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
diskq: reliable: use LogQueue API for modifying memory_usage counter
bf6fcd2 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
log-queue: add method for add/sub/inc/dec queued_messages counter
cef8c0f 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
logqueue-fifo: use LogQueue API for modifying queued_messages counter
874d51b 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
diskq: use LogQueue API for modifying queued_messages
3c2158d 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
logqueue: track stats counters internally
3d8806f 
Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
Copy link
Collaborator Author

lbudai commented Sep 21, 2018

@bazsi: I think this is what we've talked about

  • track the counters internally, when one of the counters changed, change the 'cached' value
  • counter reg: increase with the cached value
  • counter unreg: decrease with the cached value

( unit tests are missing, will added after you tried it)

@kira-syslogng
Copy link
Contributor

Build FAILURE

@lbudai
logqueue: use cached stats values when a counter is registered/unregi…
b3768f9 
…stered

Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai lbudai force-pushed the stats-prepare-for-multiple-queues-per-destdrv branch from 754837b to b3768f9 Compare September 21, 2018 11:27
@kira-syslogng
Copy link
Contributor

Build FAILURE

@lbudai
diskq: nonreliable: update memory_usage directly when started
4af4590 
It has to be added to the memory_usage;

-> and make it possible to remove
 * memory_usage_qout_initial_value and
 * memory_usage_overflow_initial_value
from LoqQueue

Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai
logqueue: remove unused variables
1472845 
These variables should never be part of LogQueue.

Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@kira-syslogng
Copy link
Contributor

Build FAILURE

1 similar comment
@kira-syslogng
Copy link
Contributor

Build FAILURE

@lbudai lbudai force-pushed the stats-prepare-for-multiple-queues-per-destdrv branch from 50224cb to 1472845 Compare September 21, 2018 13:09
@kira-syslogng
Copy link
Contributor

Build FAILURE

@lbudai lbudai force-pushed the stats-prepare-for-multiple-queues-per-destdrv branch from 1472845 to db5c21a Compare September 21, 2018 15:26
@kira-syslogng
Copy link
Contributor

Build FAILURE

@lbudai lbudai force-pushed the stats-prepare-for-multiple-queues-per-destdrv branch from db5c21a to 13a84a7 Compare September 21, 2018 16:06
@lbudai
Copy link
Collaborator Author

lbudai commented Sep 21, 2018

@bazsi: I checked again why tests has failed, and I think in case of diskq we cannot rely on a cached queued_messages value after the reload.
We store the LogQueue instance in the persist-config, this is why it work for in-memory LogQueue.
In case of diskq when we found a LogQueue instance in the persist config, the very first thing what we do is to unref it... so the cached values are dropped. This is why the tests in Kira failed.
In case of queued_messages we have to use the log_queue_get_length but instead of set we should add it to the current value...
The cached queued_messages is still need, unless we want to implement the autoreset...

I hope that it solve all the issues.

@kira-syslogng
Copy link
Contributor

Build FAILURE

@lbudai
logqueue: use log_queue_length() instead of cached value when registe…
ff79236 
…ring ctrs

The cached value for queued_messages cannot be use after a reload.

When a LogQueue instance is found the very first thing what are doing
with that is unref (in case of diskq).

On the other hand, we need to track queued_messages to ensure that the
StatsCounter is 0 after a deinit (without the autoreset feature).

Signed-off-by: Laszlo Budai <stentor.bgyk@gmail.com>
@lbudai lbudai force-pushed the stats-prepare-for-multiple-queues-per-destdrv branch from 13a84a7 to ff79236 Compare September 21, 2018 16:47
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@lbudai lbudai changed the title [wip] Stats prepare for multiple queues per destdrv Stats prepare for multiple queues per destdrv Sep 21, 2018
@bazsi
Copy link
Collaborator

bazsi commented Sep 23, 2018

I have now rebased my patches against this series, and code-wise it seems to have worked ok. I am yet to do functional testing.

At the end I've pushed the DROPPED counter down into log_queue_register_stats_counters(), the
dropped counter felt completely out of place, and we'd be relying on being able to register the same
counter multiple times anyway.

This way both the driver and the associated LogQueue would grab the same counter, the only
risk is that the StatsClusterKey/stats_level would somehow be different for the two registrations. However both of these values are actually passed in as arguments.

This is the patch I am referring to: be8a4bb

@lbudai
Copy link
Collaborator Author

lbudai commented Sep 23, 2018

@bazsi:
I'm ok with this change.

Should I add this to my PR? (if it causes conflict, I'd not touch it...)

@bazsi
Copy link
Collaborator

bazsi commented Sep 24, 2018

I have pushed that change to the bottom of my branch, so please do cherry-pick it over. I still have some rebasing work to do.

This is the one: 551dc838293146345f2c6cdf92fdd20e05f2221f

@bazsi @lbudai
logqueue: drop log_queue_set_dropped_counter()
c8fbece 
The stats subsystem is now capable of shared registrations, so we can simply
push the dropped counter registration to LogQueue, even if the parent
LogPipe element registers that as well.

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
@lbudai
Copy link
Collaborator Author

lbudai commented Sep 24, 2018

@bazsi: ok, cherry-picked.

@lbudai lbudai requested review from furiel and bazsi September 24, 2018 07:25
@lbudai
Copy link
Collaborator Author

lbudai commented Sep 24, 2018

@bazsi: I think this could to master (please don't forget to approve the PR :) )

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@bazsi bazsi mentioned this pull request Sep 24, 2018
Merged
@furiel furiel merged commit e843f79 into syslog-ng:master Sep 24, 2018
@lbudai lbudai added this to the OSE 3.18 milestone Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bazsi bazsi bazsi approved these changes

@furiel furiel furiel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
OSE 3.18
Development

Successfully merging this pull request may close these issues.
4 participants
@lbudai @kira-syslogng @bazsi @furiel