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

feature: prometheus plugin apisix_http_status metric route tag Improve recognition #1574

Closed
Donghui0 opened this issue May 12, 2020 · 14 comments · Fixed by #2497
Closed

feature: prometheus plugin apisix_http_status metric route tag Improve recognition #1574

Donghui0 opened this issue May 12, 2020 · 14 comments · Fixed by #2497
Assignees
Labels
good first issue Good for newcomers

Comments

@Donghui0
Copy link
Contributor

Issue description

metric like this:
apisix_http_status{code="200",route="00000000000000000030",service="",node="10.30.30.35"}
The value of the route tag needs to be changed to the real route value instead of route id.
and add host field.
like this:
apisix_http_status{code="200",route="/*", host="www.apisix.com", node="10.30.30.35"}

It is more convenient to query the QPS of the specified route of the domain name

@moonming
Copy link
Member

agreed, welcome PR for this

@membphis membphis added beginner good first issue Good for newcomers labels May 12, 2020
@Donghui0
Copy link
Contributor Author

When there are multiple values ​​for uris, how does the dispatch method in the lua-resty-radixtree library return which match is successful?
for example:
local rax = radix.new({ { paths = {"/aa/*", "/bb/cc/*", "/dd/ee/index.html"}, metadata = "metadata /aa", } }) rax:match("/bb/cc/xx", opts)
i want to get "/bb/cc/*"

@membphis
Copy link
Member

membphis commented Jun 4, 2020

I do not think that is a good way. I think we can add a new field to store the matched uri.

@xxm404
Copy link
Contributor

xxm404 commented Jun 9, 2020

Modify Prometheus plugin to add a new Uri field? @membphis

@membphis
Copy link
Member

it depends on this feature at lua-resty-radixtree: api7/lua-resty-radixtree#41

@swayamraina
Copy link
Contributor

@membphis is this still open for dev?

@tzssangglass
Copy link
Member

pls assign this to me, i will solve it on October 20.

tzssangglass added a commit to tzssangglass/apisix that referenced this issue Oct 16, 2020
…prove recognition apache#1574

fix apache#1574

the code is done.
todo: Modifying test cases
tzssangglass added a commit to tzssangglass/apisix that referenced this issue Oct 16, 2020
tzssangglass added a commit to tzssangglass/apisix that referenced this issue Oct 16, 2020
@tzssangglass
Copy link
Member

Under the condition of setting uris or hosts, I want to record the specific elements in the uris or hosts that match the current request.
At first I tried to take advantage of the pass by reference feature of the table, so I wrote the following code

    handler = function (api_ctx, curr_req_matched_record)
        api_ctx.matched_params = nil
        api_ctx.matched_route = route
        --to keep the record which matches the current request and pass it to api_ctx
        api_ctx.curr_req_matched_record = curr_req_matched_record
    end
    local curr_req_matched_record = {}
    match_opts.matched = curr_req_matched_record

    local ok = uri_router:dispatch(api_ctx.var.uri, match_opts, api_ctx, curr_req_matched_record)

I want to init match_opts.matched , so that I can get the matched record on the callback after dispatch success.

But this can lead to failure in some cases, because of the following code (https://github.com/api7/lua-resty-radixtree/blob/master/lib/resty/radixtree.lua#L456-L458),

    if not opts.matched and not route.param then
        return true
    end

In my case it won't return true.

So this line of thinking tends to be invalid.

I also considered the metadata you mentioned above, but that doesn't solve uris or hosts.

Now I don't have a solution, is there a way to get the specific elements in the uris or hosts that match the current request in apisix?

@membphis
Copy link
Member

you can take a look at this, it can help you:

https://github.com/api7/lua-resty-radixtree#synopsis

        -- try to match
        local opts = {
            host = "foo.com",
            method = "GET",
            remote_addr = "127.0.0.1",
            vars = ngx.var,
        }
        ngx.say(rx:match("/aa", opts))

        -- try to match and store the cached value
        local opts = {
            host = "foo.com",
            method = "GET",
            remote_addr = "127.0.0.1",
            vars = ngx.var,
            matched = {}
        }
        ngx.say(rx:match("/name/json/foo/bar/gloo", opts))
        ngx.say("name: ", opts.matched.name, " other:", opts.matched.other)

@membphis
Copy link
Member

membphis commented Oct 19, 2020

@tzssangglass
Copy link
Member

tzssangglass commented Oct 19, 2020

I saw the example above, init match_opts.matched is probably the right way to think about it, as https://github.com/api7/lua-resty-radixtree#synopsis, I did.

    match_opts.method = api_ctx.var.request_method
    match_opts.host = api_ctx.var.host
    match_opts.remote_addr = api_ctx.var.remote_addr
    match_opts.vars = api_ctx.var

    match_opts.matched = {}
    local ok = uri_router:dispatch(api_ctx.var.uri, match_opts, api_ctx)

But in this test case

=== TEST 6: hit routes: /hello1
--- request
GET /hello1
--- response_body
hello1 world
--- no_error_log
[error]

It will cause the test to fail. The reason is the following code:

        if match_route_opts(route, opts, args) then
            -- log_info("matched route: ", require("cjson").encode(route))
            -- log_info("matched path: ", path)
            if compare_param(path, route, opts) then
                if opts_matched_exists then
                    opts.matched._path = route.path_org
                end
                return route
            end
        end

https://github.com/api7/lua-resty-radixtree/blob/6b5d65eb9853c91b1d23cc4cea8941be74bb5f20/lib/resty/radixtree.lua#L663-L666

case 1: did't init match_opts.matched

The test case will success, because not opts.matched and not route.param = true .

The logic goes as follows

image

case 2: init match_opts.matched

The test case will fail, because not opts.matched and not route.param = false, then execute the following code in the function compare_param .

The logic goes as follows

image

So in this test case, init matched will cause a 404, is this to use matched and metedate in combination? But I understand that matched should always work, and metedate is used to tag the url pass of the restful api.

@tzssangglass
Copy link
Member

In radixtree# _match_from_routes function, may be judgement if it's a prefix matching before calling the compare_param function, and if so, simply return.

like this

        if match_route_opts(route, opts, ...) then
            -- log_info("matched route: ", require("cjson").encode(route))
            -- log_info("matched path: ", path)

            --judgement if it's a prefix matching
            local last_char = string.sub(route.path_org, #route.path_org)
            ngx.log(ngx.WARN, "last_char: ", last_char)
            if last_char and last_char == '*' then
                if opts_matched_exists then
                    opts.matched._path = route.path_org
                end
                return route
            end

            if compare_param(path, route, opts) then
                if opts_matched_exists then
                    opts.matched._path = route.path_org
                end
                return route
            end
        end

        ::continue::

@membphis
Copy link
Member

@tzssangglass welcome PR to fix this bug.

if we need to https://github.com/api7/lua-resty-radixtree , welcome to create a new issue first at radixtree.

@tzssangglass
Copy link
Member

@tzssangglass welcome PR to fix this bug.

if we need to https://github.com/api7/lua-resty-radixtree , welcome to create a new issue first at radixtree.

ok, i try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
6 participants