Added unit section to status endpoint #928

[Pavlusha311245]

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: https://github.com/nginx/unit/issues/928

[ac000]

Hi @Pavlusha311245

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

Closes: https://github.com/nginx/unit/issues/928

Cheers, Andrew

[Pavlusha311245]

@ac000 Do you need a commit message like this?

Add unit section to status endpoint

Added unit section to /status endpoint. Unit section is about web-server version, config last load time and config update generation
Closes: https://github.com/nginx/unit/issues/928

[ac000]

@ac000 Do you need a commit message like this?

Hi.

Add unit section to status endpoint

Added unit section to /status endpoint. Unit section is about web-server version, config last load time and config update generation
Closes: https://github.com/nginx/unit/issues/928

Just about!. Something like this at the minimum

Add unit section to status endpoint                                             
                                                                                
Added unit section to /status endpoint. Unit section is about web-server        
version, config last load time and config update generation.                    
                                                                                
<Insert example output from /endpoint here...>                                  
                                                                                
Closes: https://github.com/nginx/unit/issues/928

Wrapping lines around the 72/73 charcter mark.

Thanks!

[ac000]

Thanks for that, quick couple of questions.

"unit": {
    "version": "1.32.0",
    "load_time": "2024-01-25T13:24:08.000Z",
    "generation": 0
  },

Is load_time when unit was started or last configured?

What does generation represent?

[Pavlusha311245]

Thanks for that, quick couple of questions.

"unit": {
    "version": "1.32.0",
    "load_time": "2024-01-25T13:24:08.000Z",
    "generation": 0
  },

Is load_time when unit was started or last configured?

What does generation represent?

load_time shows when last configured generation represents count of config updates. When you update listeners, applications and etc.

[Pavlusha311245]

Some changes are ready after review. I will push it asap

[Pavlusha311245]

@ac000 Can you help me with changes.xml or I can push my version with new commit?

[ac000]

If you have it split out locally, you can force push it here with git push -f ...

[Pavlusha311245]

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...

From review context

Pardon. Should I merge with commit and push or push with new commit?

[ac000]

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.

[ac000]

Thanks for that, quick couple of questions.

"unit": {
    "version": "1.32.0",
    "load_time": "2024-01-25T13:24:08.000Z",
    "generation": 0
  },

load_time shows when last configured generation represents count of config updates. When you update listeners, applications and etc.

Hmm, we probably need a better name for this. 'last_configured' or somesuch...

Although a 'start_time' would perhaps be useful.

[Pavlusha311245]

I can add it. Ask me if you know how to name it better. Do I need add start_time?

[ac000]

I can add it. Ask me if you know how to name it better. Do I need add start_time?

My current suggestion is 'load_time' -> 'last_configured' but maybe others will have better suggestions. Don't worry about that one for now...

[hongzhidao]

Hi @Pavlusha311245 @ac000 A reference https://nginx.org/en/docs/http/ngx_http_api_module.html#example

[hongzhidao]

I can add it. Ask me if you know how to name it better. Do I need add start_time?

My current suggestion is 'load_time' -> 'last_configured' but maybe others will have better suggestions. Don't worry about that one for now...

It looks good to contain config for the option.

[hongzhidao]

@Pavlusha311245 About the time stuff, I'd suggest you look at these similar cases. https://github.com/nginx/unit/blob/master/src/nxt_http_variables.c#L334 https://github.com/nginx/nginx/blob/master/src/http/ngx_http_variables.c#L2424

[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?

[hongzhidao]

Is there a problem with the formatting or the coding itself?

   time(&rawtime);
    timeinfo = gmtime(&rawtime);  //convert to UTC
    strftime((char*)buffer, 25, "%Y-%m-%dT%H:%M:%S.000Z", timeinfo);

I mean there is nxt_time_string_t in Unit, maybe you can use it instead of calling strftime().

[Pavlusha311245]

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 🙂

[Pavlusha311245]

Is there a problem with the formatting or the coding itself?

   time(&rawtime);
    timeinfo = gmtime(&rawtime);  //convert to UTC
    strftime((char*)buffer, 25, "%Y-%m-%dT%H:%M:%S.000Z", timeinfo);

I mean there is nxt_time_string_t in Unit, maybe you can use it instead of calling strftime().

But... How to use nxt_time_strint_t? I use function nxt_conf_set_member_string in nxt_status to show data

[hongzhidao]

Is this all the comments on the code for now?

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.

How to use nxt_time_strint_t?

I'd suggest you look into the source code, it's too detailed, but I usually look at its usage by doing something like grep nxt_time_strint_t.

[ac000]

Hi @hongzhidao

Can a nxt_time_string_t store an arbitrary time?

It's not immediately obvious to me as the handler function seems to basically take a struct timespec and struct tm representing the current time.

[hongzhidao]

Can a nxt_time_string_t store an arbitrary time?

Yes, I think so.

Here's an example from nxt_app_log.c:

static nxt_time_string_t  nxt_log_debug_time_cache = {
    (nxt_atomic_uint_t) -1,
    nxt_log_debug_time,
    "%4d/%02d/%02d %02d:%02d:%02d.%03d ",
    nxt_length("1970/09/28 12:00:00.000 "),
    NXT_THREAD_TIME_LOCAL,
    NXT_THREAD_TIME_MSEC,
};


static u_char *
nxt_log_debug_time(u_char *buf, nxt_realtime_t *now, struct tm *tm, size_t size,
    const char *format)
{
    return nxt_sprintf(buf, buf + size, format,
                       tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday,
                       tm->tm_hour, tm->tm_min, tm->tm_sec,
                       now->nsec / 1000000);
}


demo()
{
   nxt_time_string_t  *time_cache;
   u_char             msg[NXT_MAX_ERROR_STR];

    thr = nxt_thread();

    time_cache = &nxt_log_debug_time_cache;

    p = nxt_thread_time_string(thr, time_cache, msg);
}

basically take a struct timespec and struct tm representing the current time.

Correct. And nxt_thread_time_string() handles the cached current time well. I think we can use it to represent string-time like the structure nxt_time_string_t naming.

The other example is related to http variable.

static nxt_int_t
nxt_http_var_time_local(nxt_task_t *task, nxt_str_t *str, void *ctx, void *data)
{
    nxt_http_request_t  *r;

    static nxt_time_string_t  date_cache = {
        (nxt_atomic_uint_t) -1,
        nxt_http_log_date,
        "%02d/%s/%4d:%02d:%02d:%02d %c%02d%02d",
        nxt_length("31/Dec/1986:19:40:00 +0300"),
        NXT_THREAD_TIME_LOCAL,
        NXT_THREAD_TIME_SEC,
    };
"src/nxt_http_variables.c" line 344 of 778

[hongzhidao]

Hi @Pavlusha311245, I'd suggest you change nxt_http_var_time_local() to meet the new string format and you can test it with access log. After you check it works, you can do it in your way with nxt_time_string_t.

[ac000]

@hongzhidao

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 that may never be used (e.g if /status is never looked at).

At least I think it might be best to simply store either a time_t or struct timespec (depending on required resolution) and then use that at the time of showing /status.

That's the bit I'm not sure about, if you can use an arbitrary struct timespec with nxt_time_string_t?

Failing that just use the standard libc API at the time of showing /status

That way you do less work at start up (only storing a time_t or struct timepsec)

You also use a little less memory for each timestamp, 8 or 16 bytes vs 25 or whatever.

[ac000]

@hongzhidao

I'd suggest you change nxt_http_var_time_local() to meet the new string format and you can test it with access log.

Are you suggesting to change the format of a timestamp in the access_log?

If so, I'm not sure that's a good idea, people may be relying on the existing format...

[hongzhidao]

Are you suggesting to change the format of a timestamp in the access_log?

Nope, it's for learning the usage of nxt_time_string_t for anyone who is not familiar with it. It's a test to do it easily. It's ok for him to write a new one directly like this requirement.

[hongzhidao]

That's the bit I'm not sure about, if you can use an arbitrary struct timespec with nxt_time_string_t?

I think so. Here's the related source code. https://github.com/nginx/unit/blob/master/src/nxt_time.c#L25

At least I think it might be best to simply store either a time_t or struct timespec (depending on required resolution) and then use that at the time of showing /status.

Sorry that I don't understand it.

[hongzhidao]

Hi @lcrilly, We could consider supporting this variable. It's a bit to do with this requirement. Both of them have the same time format. https://github.com/nginx/unit/discussions/1118

[ac000]

That's the bit I'm not sure about, if you can use an arbitrary struct timespec with nxt_time_string_t?

I think so. Here's the related source code. https://github.com/nginx/unit/blob/master/src/nxt_time.c#L25

Yes, but how do you get that into the nxt_time_string_t handler function?

The handler function is called from the likes of nxt_thread_time_string which uses the current time.

So I can't see how you could store a struct timespec and then at some point later use it with nxt_time_string_t...

Looks like you'd need to create a new function for that case...

At least I think it might be best to simply store either a time_t or struct timespec (depending on required resolution) and then use that at the time of showing /status

Sorry that I don't understand it.

Rather than storing a string representing the timestamp, you simply store a time_t or struct timespec instead, saving memory and then you only do the conversion into a string when required.

But I suppose it's a toss up between doing perhaps unnecessary work at startup and how often /status is called...