Opened 10 years ago

Closed 9 years ago

#170 closed task (fixed)

document how stats are collected (via spec files)

Reported by: larissas Owned by: naokikambe
Priority: medium Milestone: 06. 4th Incremental Release
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 2.12 Internal?: no

Description (last modified by larissas)

document how stats are collected (via spec files)

from backlog task 57

(Assigned to both Kazunori Fujiwara and Naoki Kambe)

Subtickets

Change History (30)

comment:1 Changed 10 years ago by larissas

  • Description modified (diff)

comment:2 Changed 9 years ago by shane

Kambe-san,

This is currently marked as being for the 6-month milestone. This is much too late, since we should have working statistics by then. :)

What I am not sure is when this item is expected to be done. Is this something that can be done in this release, which ends next Monday? If so, I will add it to this release. Otherwise I will move it to the following release.

Thanks!

comment:3 Changed 9 years ago by naokikambe

Shane-san,

What I am not sure is when this item is expected to be done. Is this
something that can be done in this release, which ends next Monday? If so,
I will add it to this release. Otherwise I will move it to the following
release.

No, it isn't. So please move it to the following release.
(Does it ends at end of July?)

I sent two emails about ideas related to statistics specification
until last few weeks.

https://lists.isc.org/mailman/htdig/bind10-dev/2010-May/000880.html
https://lists.isc.org/mailman/htdig/bind10-dev/2010-May/000919.html

I will write this document based on my these ideas at first. Is that OK?

Additionally I have some questions about this.

  • Is format of this document already fixed? If you have some templates for it, please let me know.
  • Where do I need to write it? on the track wiki or on the subversion repository?

Thanks in advance,

comment:4 Changed 9 years ago by naokikambe

Replying to naokikambe:

  • Is format of this document already fixed? If you have some templates for it, please let me know.
  • Where do I need to write it? on the track wiki or on the subversion repository?

I found the comlete document about config manager.

trunk/src/lib/config/document/documentation.txt

I'll make it an example of stats document.

comment:5 Changed 9 years ago by naokikambe

  • Status changed from new to reviewing

I published a draft version of the document as a wikipage here,
http://bind10.isc.org/wiki/StatsModule and now it is reviewed by
Fujiwara. So I change status of the ticket to reviewing. But I think
it is better that another person will also review it later. Because
Stats module will have relation with almost other modules in the
future.

comment:6 Changed 9 years ago by naokikambe

First, I apologize for my poor English skills, but I reply to
Fujiwara's comment. His original comments is in bind10-dev list:
https://lists.isc.org/pipermail/bind10-dev/2010-June/000982.html

|+------ Sending modules -----+

+------+ +------+ +------+ |
| Boss | | Auth | | etc. | | <- *1
+------+ +------+ +------+ |

|+-------------------------+
| | | |
| +--[CC protocol]--+
| | <- *2
| v
| +--------------+
| | Stats | <- *3
| +--------------+
| | <- *4
| v
| +-----------------+
| | Cmd-Ctrld | <- *5
| +-----------------+
|
|*1 Modules except Boss and Auth, which send stats data to stats module,
| is not supported in initial version of stats module

No, it has nothing to do with the statistics specification.

For example, statistics information related other modules, like xfrin
or xfrout, may be required in the near future release. But only Boss
and Auth modules may be supported in initial version.

|== Procedure of stats module ==
|=== Basic procedure ===
| * Initial process:
| 0. Boss starts stats daemon and other modules.
| * Main process in loop:

First, "statistics module" contacts config manager.

Configuration changes and commands from bindctl are come from config
manager.

That's right, so I already described about it following section in the
document.

| 1. Stats starts to subscribe in stats channel.
| 1. Other modules send stats data to stats module periodically.
| 1. Stats module collects data and then aggregates it.
| 1. When print_stats command is invoked via bindctl, stats daemon
| reports formatted statistics data via bindctl.
| * Final process:
| X. When Boss is shutting down, stats module and other modules are
| killed.

|== Collecting items ==
|Stats module collects following items from Boss and Auth.
| * In general (for both modules)
| * version -- A version number of this stats data definition

version is not necessary in the protocol because the version number
will be written in *.spec configuration file.

This item may be optional and not always required. I don't know
whether it is necessary or not now. But it may be needed to check the
format validation when the definition of the format will have changed
in the future release. If the version of the format mismatches between
the sender and receiver, data may be dropped by receiver.

| * module -- A module name which sends the stats data
| * process_id -- A process id of the module

process_id is not used in another part of BIND 10.
So, local name defined in msgq may be better.

I think process_id is not so useful item, but it may be simple
information for initial statistics features.

| * processes -- A number of processes of the same module, if
| multiple processes of the module are running

then, "process_id" may be a list of processes.

"process_id" is not a list but a number of process id of a existent
process.

| * send_time -- Milli-seconds of current time since epoch time
| (1970-01-01T00:00:00Z)

why milli second?
I prefer unixtime + microsecond (struct timeval) format.

I adopted JSON schema for definition of the statistics data in BIND

  1. It's described in the Internet-Draft

http://tools.ietf.org/html/draft-zyp-json-schema . "utc-millisec" is a
format described in it.

What are "T" and "Z" characters?
Text printable format is hard to parse.

"YYYY-MM-DDThh:mm:ssZ" is ISO-8601 format. "T" is a delimiter between
date and time, "Z" means UTC. Because of standard format, it may not
be so hard to parse.

| * sequence -- A sequence number which must be unique and consistent
| in the sending module

Is this necessary?

I think it's necessary. Because of packet loss or something, if
sequence of data of the sender is wrong, the receiver can detect it is
wrong data.

| * For Boss module
| * boot_time -- A date time when BIND 10 starts up, format is
| YYYY-MM-DDTHH:MM:SSZ
| * For Auth module
| * queries_in
| * tcp -- A number of query counts per a process which Auth servers receives in
| TCP since it sends last
| * udp -- A number of query counts per a process which Auth servers receives in
| UDP since it sends last

CC module has good counters.

In current version of CC module, it doesn't seem to have counters.
But if yes, it is good items for statistics.

|== Reporting items ==
|Stats module reports following items via bindctl.

This format will be generated by parsing each *.spec file.

May spec file for stats module include a template of output format of
stats data?

| * Local name -- A localname, which is returned from msgq module in CC protocol

Local name is assigned for each process.

| * Boot time -- A date time when BIND 10 process starts
| * Reported time -- A date time when stats module reports
| * Process id -- Process ids of all related modules
| * Incoming Queries (TCP) -- A calculated query counts by stats module
| * Incoming Queries (UDP) -- A calculated query counts by stats module
|
|This is an example of output image via bindctl.
|{{{
| ++ BIND 10 Statistics Report ++
| Local name: 4bea7903_4@host
| Boot time: 2010-05-13T05:19:43Z
| Report time: 2010-05-13T05:44:41Z
| Process id(Boss): 777
| Process id(Auth): 888
| Process id(Stats): 999
| Incoming Queries (TCP): 8888
| Incoming Queries (UDP): 9999
| ++ BIND 10 Statistics Report ++
|}}}

The output format requires another knowledge.
The statistics module only knows input data format.
Or we must define output data format definition.

My idea was:

  • BIND 10 Statistics report Report time: ...

bind10.LocalName?: xxxx@localhost
bind10.BootTime?: ...

auth.LocalName?: xxx
auth.queries.tcp: ...
auth.queries.udp: ...

This is also good idea. I think output format via bndctl should be
human friendly and easy changeable for administrator, and should require
less another knowledge. We must decide the best format for
it. Property names printed via bindctl may be defined in somewhere.

|== Available commands in bindctl ==
|Two commands via bindctl are available in initial version of stats
|module.
| * "print_stats" command:
| Stats module aggregates current numbers and prints the list of
| them by using formatted text.

print_stats command may have module name arguments.
print_status without arguments show all statistics.

I think it may require no arguments of this command in initial version
of stats module. Because output data should be summarized by stats module.
But somebody wants to know stats data of a specific module for
debugging or something.

| * "print_clear" command:

typo. It is "clear_statistics".

Yes, "print_clear" command makes no sense. I'll change this name to
"clear_stats".

| Stats module resets query counts to zero. If this command is
| invoked, then at first 'Are you sure?' prompt to confirm it.

The command may also have module name arguments.

This command clears only two query counters. It doesn't require
argument of module name in initial version.

|== Backend DB for stats module ==
|(TBD)
|A specific DB, like sqlite3 or Berkeley DB, is not used in stats
|module in initial version. It's assumed that stats module keeps
|aggregated data in memory.

It is not defined, I think.

It is simple. Stats module in initial version requires no specific
Backend DB. It may store data only in python variables. It may depend
on python runtime.

|== Message format ==
|Message format from Boss module to Stats:
|{{{
|#!js
|{
| "stats_data":
| {
| "General":
| {
| "version": "1.0",

It is not necessary.

| "module": "Auth",

The parameter may be included in the data itself.

It's my mistake. "Boss" is correct. This item is required because
stats module don't know which module sends data.

| "process_id": 777,

The localname in the envelope is sufficient.

| "send_time": "2010-05-13T05:40:41Z",

It may be included in the data itself.

I think the time when stats data departs from the module may
required. Because only sender module knows it.

| "sequence": 2345,

It is not necessary.

| },
| "Boss":

Is it a mistake? Is it "Auth:"?
But It contains module name.

This is correct, this property is equivalent to the above module
name. Stats module reads "module name" item above and then it follows
here. Otherwise, it doesn't know which module stats data comes
from. If lack of this item, it's too difficult to express data schema
of stats data, so it may be too difficult for stats module to validate
coming data.

Then "General" section is not necessary.

Yes, "General" section may be disable and items in it may be moved to
upper level.

|Message format from Auth module to Stats:
|{{{
|#!js
|{
| "stats_data":
| {
| "General":
| {
| "version": "1.0",
| "module": "Auth",
| "process_id": 888,
| "processes": 2,
| "send_time": "2010-05-13T05:40:41Z",
| "sequence": 2345,
| },
| "Auth":
| {
| "queries_in":
| {
| "tcp" : 123,
| "udp": 4567
| }
| }
| }
|}
|}}}

To simplify the format, I propose a new data format.
Add a "timestamp" as a unixtime in the outermost.

From or Local name is obtained from envelope.

Module name is "Auth" which exists in the data.

I think at least items such as "module", "process_id", "send_time",
and "sequence" may be required. I mentioned the reason above.

{

"timestamp": unixtime,

| "Auth":
| {
| "queries_in":
| {
| "tcp" : 123,
| "udp": 4567
| }
| }

}

|== Data schema ==
|A schema which defines above massage formats, filename of which is configured in
|spec file for stats module.
|stats_data_schema.spec:
|{{{
|#!js
|{
| "stats_data":
| {
| "description": "A schema for BIND 10 stats data definitions \
| using JSON schema syntax (http://json-schema.org/)",
| "type": "object",


I prefer "dict".

"object" is a type defined in JSON schema. See the internet-draft
above.

The "stats_data" schema is defined for each module.
I prefer it will be written in *.spec file.

I prefer new spec file format will be:

{ "module_spec": { ... },

"commands": { ... },
"stats_spec": { ... }

}

The "stats_spec" format should be compatible with "module_spec" and
"commands" format.

I'm afraid that spec file is very long if it also contains all stats
data definitions in the spec file. It depends on the implementation of
config manager, so config manager may need to parse this new item.
Besides, if spec files for each module contain stats data schema, it
may become very verbose because each module share the schema and same
schema may be written in spec file for each module.

comment:7 follow-up: Changed 9 years ago by naokikambe

I revised Document: How stats module collects data.

  • applied Fujiwara's comment as much as possible
  • added commands and related arguments
  • added some stats data items and deleted
  • changed some data structures and schemas (Data schemas are included in spec files in order to simplify.)
  • changed a bit explanation and expressions.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by naokikambe

Fujiwara's review is complete, so this ticket is ready for review by someone.

comment:9 Changed 9 years ago by shane

  • Owner changed from naokikambe to UnAssigned

comment:10 Changed 9 years ago by shane

  • Milestone changed from y2 6 month milestone to 05. 3rd Incremental Release: Serious Secondary

comment:11 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by shane

  • Owner changed from UnAssigned to naokikambe

Replying to naokikambe:

Fujiwara's review is complete, so this ticket is ready for review by someone.

This looks good, with one (minor) point:

This system supports "clearing" the statistics, which I think means setting them to 0. However, there is no information reporting about when or if this has been done. I think adding a timestamp to the values reported by the auth server and the boss process which is the time when statistics were cleared might be useful. This would be the start time of the process initially.

comment:12 follow-up: Changed 9 years ago by jinmei

I have no substantial comments on the documents.

While reviewing it I've noticed a few minor editorial nits and directly fixed them on the wiki page. Please check the changes.

The only technical comment of mine on the description is about this part:

  • tcp -- A number of query counts per a process which Auth servers receives in TCP since it sends last
  • udp -- A number of query counts per a process which Auth servers receives in UDP since it sends last

Does this mean the auth server records counter values last time it reported to some other modules and only report the diff from the previous value? I'm not sure if this is a good behavior. If the "previous" value is lost in some way, the receiever can never calculate the total number of that counter. Also, what if the auth server reports multiple instances of stats-(like) modules? Does the auth server have to keep the state of the "previously reported value" for all possible clients? I think it's much simpler and more flexible if the auth server just reports the total number of each counter and have the receiever be responsible for calculating a diff between two reports (if necessary).

comment:13 Changed 9 years ago by jinmei

  • Milestone changed from 05. 3rd Incremental Release: Serious Secondary to 06. 4th Incremental Release

comment:14 in reply to: ↑ 11 Changed 9 years ago by naokikambe

  • Owner changed from naokikambe to shane

Replying to shane:

Thank you for your comment.

This system supports "clearing" the statistics, which I think means
setting them to 0. However, there is no information reporting about when
or if this has been done. I think adding a timestamp to the values
reported by the auth server and the boss process which is the time when
statistics were cleared might be useful. This would be the start time of
the process initially.

I'll add description like this to this document:

The stats module always reports "stats.BootTime", "stats.StartTime"
and "stats.LastUpdateTime. "stats.BootTime" means the time when the
stats module starts initially or the time when the stats module
restarts. "stats.StartTime" means the time when the stats module
starts collecting data and aggregating values and the stats module
doesn't reset the value of "stats.StartTime" to current time until
"clearing" command is done. "stats.LastUpdateTime" means the latest
time when another module like auth server or boss process reports to
stats module.

I think these reporting items is useful for identifying when or if
"clearing" has been done.

Is that OK? If OK, I'll revise this document, otherwise I welcome
better idea than this.:)

comment:15 in reply to: ↑ 12 Changed 9 years ago by naokikambe

Replying to jinmei:

Thank you for your correcting my typos and your comments.

  • tcp -- A number of query counts per a process which Auth servers

receives in TCP since it sends last

  • udp -- A number of query counts per a process which Auth servers

receives in UDP since it sends last

Does this mean the auth server records counter values last time it
reported to some other modules and only report the diff from the previous
value? I'm not sure if this is a good behavior. If the "previous" value
is lost in some way, the receiever can never calculate the total number of
that counter. Also, what if the auth server reports multiple instances of
stats-(like) modules? Does the auth server have to keep the state of the
"previously reported value" for all possible clients? I think it's much
simpler and more flexible if the auth server just reports the total number
of each counter and have the receiever be responsible for calculating a
diff between two reports (if necessary).

These counters means "total" count for one auth server, but I
think "incremental" count for one auth server may be also supported.
Because I think each auth sever may not know how many auth servers
work and if it means total counter, all auth servers must support a
very long-range counter. I think it may be very heavy for auth servers.
However I also think supporting only total count for all servers is
easier and more reasonable for the auth server and the stats module.

So, the stats module(receiver) supports two methods of adding value
and replacing(setting) value for the sender, so that the sender
including the auth server can freely choice increment or replace the
counter values in the stats module, and also the sender freely add
collecting items.

  • A example of "adding" method:

If the sender sends this message to the receiver via bindctl,

{ "command" :  [ "add", { "auth.queries.udp" : 9876 } ]}

the receiver calculates like this:

data["auth.queries.udp"] = data["auth.queries.udp"] + 9876
  • A example of "replacing(setting)" method:

If the sender sends this message to the receiver via bindctl,

{ "command" :  [ "set", { "auth.queries.udp" : 9876 } ]}

the receiver calculates like this:

data["auth.queries.udp"] = 9876
  • A example of "adding new collecting item and setting value" method:

There is no difference from "setting" method. If the sender sends this
message to the receiver via bindctl,

{ "command" :  [ "set", { "auth2.queries.udp" : 5432 } ]}

the receiver calculates like this:

data["auth2.queries.udp"] = 5432

A variable "data" is initially defined as dictionary type.

What about this idea? If it's no problem for the auth server, I'll
revise this document, otherwise I welcome better idea than this.:)

comment:16 Changed 9 years ago by naokikambe

  • billable set to 0
  • Estimated Difficulty set to 0.0
  • Internal? unset

I've updated this document StatsModule. I adopted given above 2
comments and added parts of data definition and supported methods via
bindctl and using msgq library according to current implementation of
trac #191. The status of this ticket remains unchanged(reviewing).

comment:17 follow-up: Changed 9 years ago by shane

  • Owner changed from shane to jinmei

From my point of view this is fine. Jinmei should address the concerns directed to him. I'm changing the owner so he can do that (sorry about not doing this sooner!).

comment:18 in reply to: ↑ 17 ; follow-ups: Changed 9 years ago by jinmei

Replying to shane:

From my point of view this is fine. Jinmei should address the concerns directed to him. I'm changing the owner so he can do that (sorry about not doing this sooner!).

Ditto about not doing this sooner.

I'm responding to this comment: https://bind10.isc.org/ticket/170?replyto=17#comment:14

These counters means "total" count for one auth server, but I
think "incremental" count for one auth server may be also supported.
Because I think each auth sever may not know how many auth servers
work and if it means total counter, all auth servers must support a
very long-range counter. I think it may be very heavy for auth servers.

I'm not sure if I understand this part. Do you mean an auth server can run for a long time (e.g. several years) and a monotonically increasing counter may overflow? If so, I don't know how the number of auth servers (btw what this means? the number of auth server processes/threads assuming we adopt a multi-process/thread server model?) is related to the point. Also, should we really worry about overflow in a realistic scenario? Suppose we handle 100,000 qps, and have a counter of number of queries. If it's a 64bit unsigned integer, it won't overflow until 368,452,886,146 years later. Even if it's 32bit, it won't overflow until 85 years later.

If this is about overflow, and if there's no realistic scenario we should worry about, I suggest we begin only with a simple total counter. Otherwise, please explain what's I'm missing.

I have no problem with the proposed python syntax, btw.

comment:19 Changed 9 years ago by jinmei

  • Owner changed from jinmei to naokikambe

comment:20 in reply to: ↑ 18 Changed 9 years ago by jinmei

Replying to jinmei:

Suppose we handle 100,000 qps, and have a counter of number of queries. If it's a 64bit unsigned integer, it won't overflow until 368,452,886,146 years later. Even if it's 32bit, it won't overflow until 85 years later.

Sorry, I've noticed the calculation was incorrect. These should be 5,849,424 years and 0.0014 years, respectively:

>>> 2**64 / (3600 * 24 * 365 * 100000)
5849424.17355072
>>> 2**32 / (3600 * 24 * 365 * 100000)
0.0013619251953323186

(ignoring leap years)

But a 64bit counter still seems more than enough.

comment:21 in reply to: ↑ 18 ; follow-up: Changed 9 years ago by naokikambe

  • Add Hours to Ticket changed from 0.0 to 0.5
  • Owner changed from naokikambe to jinmei
  • Total Hours changed from 0.0 to 0.5

Please don't worry about not doing sooner. :)

Replying to jinmei:

I'm not sure if I understand this part. Do you mean an auth server can run for a long time (e.g. several years) and a monotonically increasing counter may overflow? If so, I don't know how the number of auth servers (btw what this means? the number of auth server processes/threads assuming we adopt a multi-process/thread server model?) is related to the point. Also, should we really worry about overflow in a realistic scenario? Suppose we handle 100,000 qps, and have a counter of number of queries. If it's a 64bit unsigned integer, it won't overflow until 368,452,886,146 years later. Even if it's 32bit, it won't overflow until 85 years later.

Replying to jinmei:

Replying to jinmei:

Suppose we handle 100,000 qps, and have a counter of number of queries. If it's a 64bit unsigned integer, it won't overflow until 368,452,886,146 years later. Even if it's 32bit, it won't overflow until 85 years later.

Sorry, I've noticed the calculation was incorrect. These should be 5,849,424 years and 0.0014 years, respectively:

>>> 2**64 / (3600 * 24 * 365 * 100000)
5849424.17355072
>>> 2**32 / (3600 * 24 * 365 * 100000)
0.0013619251953323186

(ignoring leap years)

But a 64bit counter still seems more than enough.

Yes, I was afraid about overflow of integer at first, but I now don't need to be afraid it by this proof.
It is realized that it is enough in both 32bit and 64bit integer in python. We don't care whether we should use 32bit or 64bit in python. In this stats module, the query counts are set in python variables.

The stats module wants the auth module to throw numbers of queries periodically by increase methods or set method, which are defined in spec file of stats module. The auth module can choose which methods to throw query counts. The stats module can handle it whichever methods the auth module choose.

comment:22 in reply to: ↑ 21 Changed 9 years ago by jinmei

  • Add Hours to Ticket changed from 0.0 to 0.12
  • Owner changed from jinmei to naokikambe
  • Total Hours changed from 0.5 to 0.62

Replying to naokikambe:

Please don't worry about not doing sooner. :)

Thanks, but please don't hesitate to kick me. I'm lazy:-)

But a 64bit counter still seems more than enough.

Yes, I was afraid about overflow of integer at first, but I now don't need to be afraid it by this proof.
It is realized that it is enough in both 32bit and 64bit integer in python. We don't care whether we should use 32bit or 64bit in python. In this stats module, the query counts are set in python variables.

The stats module wants the auth module to throw numbers of queries periodically by increase methods or set method, which are defined in spec file of stats module. The auth module can choose which methods to throw query counts. The stats module can handle it whichever methods the auth module choose.

I still don't get it. If we don't worry about overflow, and if the main purpose of the increase method was to work around overflow scenarios, why do we need it? If it could be implemented very easily that might not be a big deal (although we should still be careful not to add knobs that are not really necessary), but as I pointed out before, this would require the sender (typically the auth/recursive server) maintain complicated states in addition to the simply total counter. So, to me, supporting increase seems to make the implementation complicated for non existent purposes.

If the above understanding of mine is correct, I'd suggest

  • the sender (auth server, etc) only maintains a monotonically increasing "total" counter
  • we only provide the set method

at least for the initial implementation.

If, in future, we see a strong need for the increase method, we may consider how to do that at that point. It's easy to start with a minimal set of necessary features and to extend it as we see the need for it; the reverse is not easy (actually often impossible due to compatibility reasons).

comment:23 follow-up: Changed 9 years ago by naokikambe

I think we may use increase method instead of thinking of integer overflow. For example, in multi-process module, which isn't exactly auth module, each process knows its own total count but it cannot know total count of ALL processes.
In this case, I think, to calculate the total count of ALL processes, the stats module must collect the counts from such processes and summarize it. In such case, I think the increase method is useful.
However if we don't need to think this case, we can disable the increase method.

comment:24 in reply to: ↑ 23 ; follow-up: Changed 9 years ago by jinmei

Replying to naokikambe:

I think we may use increase method instead of thinking of integer overflow. For example, in multi-process module, which isn't exactly auth module, each process knows its own total count but it cannot know total count of ALL processes.

Right.

In this case, I think, to calculate the total count of ALL processes, the stats module must collect the counts from such processes and summarize it.

Right.

In such case, I think the increase method is useful.

I don't get this part. How exactly does the increase method help this scneario? Could you be more specific, maybe with an example case?

comment:25 in reply to: ↑ 24 ; follow-up: Changed 9 years ago by jinmei

Replying to jinmei:

In such case, I think the increase method is useful.

I don't get this part. How exactly does the increase method help this scneario? Could you be more specific, maybe with an example case?

I think I understand it. You probably wanted to let the receiver (stats collector) be unaware of different senders.

If so, I see the advantage. But it comes with its own cost. Basically it's a tradeoff about which one should maintain states, the sender or the receiver.

I personally still think it's better to have the receiver keep the state (e.g. a list of senders) because the sender (which is most typically some DNS server module) would tend to be pretty complicated without this and it would make sense to keep it as simple as possible. But I understand this may be a matter of opinion.

In any case, I'd suggest a minimalist approach: begin with "total" only, and when we see the real need where increase can be a choice we should discuss whether that's the best approach.

For the design documentation, I'd suggest we leave it open, describing
these discussion points.

comment:26 in reply to: ↑ 25 ; follow-up: Changed 9 years ago by naokikambe

  • Add Hours to Ticket changed from 0.0 to 1.0
  • Owner changed from naokikambe to jinmei
  • Total Hours changed from 0.62 to 1.62

Replying to jinmei:

I personally still think it's better to have the receiver keep the state (e.g. a list of senders) because the sender (which is most typically some DNS server module) would tend to be pretty complicated without this and it would make sense to keep it as simple as possible. But I understand this may be a matter of opinion.

In any case, I'd suggest a minimalist approach: begin with "total" only, and when we see the real need where increase can be a choice we should discuss whether that's the best approach.

Yes, I love simplest model too. :)
As you mentioned, increase command may be unsuitable for the auth module. But I cannot consider that deeply now. :(

In the initial version of stats module, only two modules which are auth and boss module are talk with the stats module and they don't need this command now. So it's no problem if we disable this command now. We do this. That is,

  • Stats module handles only total counts. It never handles incremental counts.
  • So we remove increase command from both the document StatsModule and the implement #191. However if in the future this command is required, we'll consider it again.

For the design documentation, I'd suggest we leave it open, describing
these discussion points.

If this conclusion is agreed, I'll close #170. If we consider that again, we'll open another ticket.

Is that OK? Please let me know.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to naokikambe

Replying to naokikambe:

  • So we remove increase command from both the document StatsModule and the implement #191. However if in the future this command is required, we'll consider it again.

For the design documentation, I'd suggest we leave it open, describing
these discussion points.

If this conclusion is agreed, I'll close #170. If we consider that again, we'll open another ticket.

Is that OK? Please let me know.

Works for me. I'd leave the considerations about increase in the documentation (rather than completely removing it), but it's up to you.

comment:28 in reply to: ↑ 27 ; follow-up: Changed 9 years ago by naokikambe

  • Add Hours to Ticket changed from 0.0 to 0.5
  • Owner changed from naokikambe to jinmei
  • Total Hours changed from 1.62 to 2.12

Replying to jinmei:

Works for me. I'd leave the considerations about increase in the documentation (rather than completely removing it), but it's up to you.

I removed the description about 'increase' command from the document and I appended the consideration in the 'TODO' section. Please see StatsModule.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to naokikambe

Replying to naokikambe:

Replying to jinmei:

Works for me. I'd leave the considerations about increase in the documentation (rather than completely removing it), but it's up to you.

I removed the description about 'increase' command from the document and I appended the consideration in the 'TODO' section. Please see StatsModule.

I already left the decision to you. The latest document is fine, please go ahead.

comment:30 in reply to: ↑ 29 Changed 9 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to jinmei:

I already left the decision to you. The latest document is fine, please go ahead.

Thank you for checking again. I'm closing it.

Note: See TracTickets for help on using tickets.