-
Notifications
You must be signed in to change notification settings - Fork 327
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
Added unit section to status endpoint #928 #1089
base: master
Are you sure you want to change the base?
Added unit section to status endpoint #928 #1089
Conversation
Thanks for the PR! OK, first things first, this is going to need a commit message. What is this commit introducing and why do we want/need it. In this case perhaps also some example output from the endpoint. Could you move the GH issue number from the commit subject and put it at the bottom of the commit message (separated by a blank line) in the form
Cheers, |
@ac000 Do you need a commit message like this?
|
.gitignore
Outdated
@@ -2,3 +2,4 @@ | |||
Makefile | |||
*.pyc | |||
__pycache__/ | |||
/.idea/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure what thiis is. However I suspect this is better handled locally your end by either adding it into your global .gitignore or in the repository under .git/info/exclude
docs/changes.xml
Outdated
<change type="feature"> | ||
<para> | ||
added unit section to /status endpoint. | ||
unit section is about web-server version, config last load time and config update generation | ||
</para> | ||
</change> | ||
|
||
|
||
<change type="feature"> | ||
<para> | ||
$request_id variable contains a string that is formed using random data and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need two of these!?
docs/changes.xml
Outdated
date="2023-08-31" time="18:00:00 +0300" | ||
packager="Nginx Packaging <nginx-packaging@f5.com>"> | ||
|
||
<change type="feature"> | ||
<para> | ||
added unit section to /status endpoint. | ||
unit section is about web-server version, control_socket and etc. | ||
</para> | ||
</change> | ||
|
||
<change type="change"> | ||
<para> | ||
if building with njs, version 0.8.0 or later is now required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here's the other one.
I would also prefer the changelog entry to be in its own commit. This can give things like git-revert(1) a better chance of working if we ever need to for some reason...
src/nxt_controller.c
Outdated
nxt_buf_t *b; | ||
nxt_port_t *main_port; | ||
nxt_runtime_t *rt; | ||
time_t rawtime; | ||
struct tm *timeinfo; | ||
u_char buffer[25]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables should be sorted in christmias tree order and variable types of the same length should be sorted alphabetically, yeah, I know, I didn't make the rule...
src/nxt_runtime.c
Outdated
nxt_sockaddr_t *sa; | ||
nxt_file_name_str_t file_name; | ||
const nxt_event_interface_t *interface; | ||
time_t rawtime; | ||
struct tm *timeinfo; | ||
u_char buffer[25]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto variables...
src/nxt_controller.c
Outdated
strftime((char*)buffer, 25, "%Y-%m-%dT%H:%M:%S.000Z", timeinfo); | ||
|
||
rt->conf_ltime.length = strlen((char*)buffer); | ||
rt->conf_ltime.start = (u_char*) malloc(rt->conf_ltime.length + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't tend to call malloc(3) directly. There is a nxt_malloc() you can use.
Also please don't cast the return, in C it simply isn't required and (in that past at least) can hide bugs.
src/nxt_runtime.c
Outdated
strftime((char*)buffer, 25, "%Y-%m-%dT%H:%M:%S.000Z", timeinfo); | ||
|
||
rt->conf_ltime.length = strlen((char*)buffer); | ||
rt->conf_ltime.start = (u_char*) malloc(rt->conf_ltime.length + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diit malloc(3).
Hmm, duplicated code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ac000 How I can fix code duplicate? I'm using this code to get time and convert it to nxt_str_t. I can create nxt_time.c with function to reuse it
time_t rawtime;
u_char buffer[25];
struct tm *timeinfo;
time(&rawtime);
timeinfo = gmtime(&rawtime); //convert to UTC
strftime((char*)buffer, 25, "%Y-%m-%dT%H:%M:%S.000Z", timeinfo);
rt->conf_ltime.length = strlen((char*)buffer);
rt->conf_ltime.start = nxt_malloc(rt->conf_ltime.length + 1);
src/nxt_status.c
Outdated
rt = task->thread->runtime; | ||
|
||
static nxt_str_t unit_str = nxt_string("unit"); | ||
static nxt_str_t ver_str = nxt_string("version"); | ||
static nxt_str_t gen_str = nxt_string("generation"); | ||
static nxt_str_t ltime_str = nxt_string("load_time"); | ||
static nxt_str_t conns_str = nxt_string("connections"); | ||
static nxt_str_t acc_str = nxt_string("accepted"); | ||
static nxt_str_t active_str = nxt_string("active"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable declarations should come before code... (I know there are some places that break this).
src/nxt_status.c
Outdated
size_t i; | ||
nxt_str_t name; | ||
nxt_str_t version; | ||
nxt_int_t ret; | ||
nxt_status_app_t *app; | ||
nxt_conf_value_t *status, *obj, *apps, *app_obj; | ||
nxt_runtime_t *rt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto variables...
Hi.
Just about!. Something like this at the minimum
Wrapping lines around the 72/73 charcter mark. Thanks! |
Thanks for that, quick couple of questions. "unit": {
"version": "1.32.0",
"load_time": "2024-01-25T13:24:08.000Z",
"generation": 0
}, Is What does |
load_time shows when last configured |
Some changes are ready after review. I will push it asap |
2e85ebd
to
9611a3b
Compare
@ac000 Can you help me with changes.xml or I can push my version with new commit? |
If you have it split out locally, you can force push it here with |
From review context Pardon. Should I merge with commit and push or push with new commit? |
After you split the changelog changes into their own commit, you should then have two commits for this PR, at which point you can force push them to your feature/unit_about_section branch. |
Hmm, we probably need a better name for this. 'last_configured' or somesuch... Although a 'start_time' would perhaps be useful. |
I can add it. Ask me if you know how to name it better. Do I need add |
strftime((char*)buffer, 25, "%Y-%m-%dT%H:%M:%S.000Z", timeinfo); | ||
|
||
rt->conf_ltime.length = strlen((char*)buffer); | ||
rt->conf_ltime.start = nxt_malloc(rt->conf_ltime.length + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing the malloc(3), but now my next question is, where does this memory get free(3)'d?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know when memory should be released. Can you explain me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use nxt_mp_xxx api instead that you don't need to explicitly do memory free.
It will be released together with the mp destroy.
src/nxt_runtime.c
Outdated
rt->conf_ltime.length = strlen((char*)buffer); | ||
rt->conf_ltime.start = nxt_malloc(rt->conf_ltime.length + 1); | ||
|
||
strcpy((char*)rt->conf_ltime.start, (char*)buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be nxt_copystr()
My current suggestion is 'load_time' -> 'last_configured' but maybe others will have better suggestions. Don't worry about that one for now... |
Added unit section to /status endpoint. Unit section is about web-server version, config last load time and config update generation Response example below: {"unit":{"version":"1.32.0","load_time":"2024-01-25T13:24:08.000Z","generation":0},"connections":{"accepted":0,"active":0,"idle":0,"closed":0},"requests":{"total":0},"applications":{"laravel":{"processes":{"running":1,"starting":0,"idle":1},"requests":{"active":0}}}} Closes: nginx#928
9611a3b
to
2cb99ca
Compare
It looks good to contain |
@Pavlusha311245 |
Hi @hongzhidao I can start working on this pr again. Can you describe in more detail what you meant by giving links to methods? Is there a problem with the formatting or the coding itself? |
I mean there is |
Is this all the comments on the code for now? I would like to know if there is anything else, then I can fully understand what to do so as not to return to the dialogue after each completed stage 🙂 |
But... How to use nxt_time_strint_t? I use function |
I'm afraid we can only point out that these are obvious first. Not sure if there is anyone else. And in our experience, it's common to rework patches after review.
I'd suggest you look into the source code, it's too detailed, but I usually look at its usage by doing something like |
Hi @hongzhidao Can a It's not immediately obvious to me as the handler function seems to |
Yes, I think so. Here's an example from
Correct. And The other example is related to http variable.
|
Hi @Pavlusha311245, |
It's a convoluted interface, but I see how it would work now. However I think it could end up doing quite a lot of work at start up At least I think it might be best to simply store either a That's the bit I'm not sure about, if you can use an arbitrary Failing that just use the standard libc API at the time of showing That way you do less work at start up (only storing a You also use a little less memory for each timestamp, 8 or 16 bytes vs |
Are you suggesting to change the format of a timestamp in the If so, I'm not sure that's a good idea, people may be relying on the |
Nope, it's for learning the usage of |
I think so. Here's the related source code.
Sorry that I don't understand it. |
Yes, but how do you get that into the The handler function is called from the likes of So I can't see how you could store a Looks like you'd need to create a new function for that case...
Rather than storing a string representing the timestamp, you simply store But I suppose it's a toss up between doing perhaps unnecessary work at |
Yes, you need a specific function, for example.
Note that the function
There are serval ways to do it like what happens in
I guess it's not often, but anyway, I feel |
[...]
My point is these all operate on the current time. So you'd need a
I wasn't asking how to do it, I was suggesting it may be a preferable
What is cached exactly? I think there's a lot over-optimisation in Unit... However reading the clock should generally be pretty efficient as it's
Where it makes sense... |
Take the error log as an example, if there are lots of logs generated in a very short term, for example, 1s, these logs may count the time only once, but not calling something like
Good to know, thanks. Either way, we have to do two things: get the current time and format time-string. |
nxt_int_t conf_gen; | ||
nxt_str_t conf_ltime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a nxt_status_report_t
structure in src/nxt_status.h
. I wonder if these would be better placed there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a
nxt_status_report_t
structure insrc/nxt_status.h
. I wonder if these would be better placed there?
Hmm... I can try 🙂 But I thought of leaving them in runtime so that I could use them anywhere else. Also I need it here to known time when config initialized. Have you suggestion how I can do it? @ac000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems they don't belong to nxt_runtime_t
and nxt_status_report_t
.
The nxt_status_report_t
is a temporary structure to collect the status information from every engine between worker threads in the router process.
The configuration generation needs to increase when a new configuration is generated. But I can't see where it happens in the PR. The configuration time needs to be recorded on its corresponding configuration.
I feel you need to handle them first.
No clear idea on it yet, but you might start with nxt_router_conf_data_handler()
which generates configuration structure through control API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration generation needs to increase when a new configuration is generated. But I can't see where it happens in the PR. The configuration time needs to be recorded on its corresponding configuration. I feel you need to handle them first.
The update of the counter and time is now located in nxt_controller
in thenxt_controller_conf_store()
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration is updated by the control API which is created in the controller process.
I feel it has nothing to do with the structure nxt_runtime_t
.
When a new configuration is applied, the last JSON value is stored as nxt_controller_conf
.
We could consider putting the generation and updated time in this structure nxt_controller_conf_t
.
Btw, debug information in error.log
is useful.
On Thu, 21 Mar 2024 06:40:46 -0700 Pavel Zavadski ***@***.***> wrote:
@Pavlusha311245 commented on this pull request.
> + nxt_int_t conf_gen;
+ nxt_str_t conf_ltime;
But I thought of leaving them in runtime so that I could use them anywhere else
Do you have a specific use case in mind?
I think it would be nice to have _all_ (or at least most of them, I
can see the odd one possibly coming from some other pre-existing
place) the status variables stored in the same place.
If you did then find that you needed them for some other purpose, then
I would look at making the status structure more accessible.
I know this may sound like a lot of extra work, but I think it's
important to get the fundamentals right now, before this API is
expanded any further...
|
@ac000 Hi. Unfortunately, I can’t think of any case, although I think it might suddenly be needed. As I understand from the message. If I need to save a given use case, will I need to make a structure for this? |
@Pavlusha311245 Hello there! If I understand correctly it looks like left to do is to use the nxt_mp_alloc instead of nxt_alloc and to refactor to extend the nxt_controller module instead of keeping the new data in the nxt_runtime_t object? Please let us know if there is anything blocking you! |
There is not enough time to complete this task. I want to close it as soon as possible so that the changes go to the main branch. If there is such a possibility, I would ask for help |
Added unit section to /status endpoint. Unit section is about web-server version, config last load time and config update generation
Response example below:
Closes: #928