You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/10/24 12:56:12 UTC
[GitHub] [apisix] xinxinh2020 opened a new pull request, #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
xinxinh2020 opened a new pull request, #8159:
URL: https://github.com/apache/apisix/pull/8159
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1032012817
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) == 1 then
Review Comment:
good advice & work done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1486570494
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1290071166
> I don't think it's a good idea to support constants on `extra_labels`, we have metrics like `apisix_node_info`, maybe we should extend it to support more constants.
As far as i am concerned, `apisix_node_info` is used to show all of the apisix info, and the `apisix_node_info` is not associated with other metrics like `http_status`.
Maybe we can add a variable like $node_info_id, so we can refer the `apisix_node_info` in other metrics.
But i still argue that adding apisix-cluster-level const in some metrics is useful. For example, we have four apisix clusters with lables:
- cluster1: deploy=vm, provider=aws, type=public
- cluster2: deploy=k8s, provider=aws, type=private
- cluster3: deploy=k8s, provider=azure, type=private
- cluster4: deploy=vm, provider=azure, type=public
Even with extended `apisix_node_info`, how can i query metrics group by any combination of these labels, such as provider=aws, or deploy=vm&type=public ?
If we tag these labels in metrics, it would be quite convenient and a promql like `apisix_http_status{provider="aws"}` would be enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1289915085
I don't think it's a good idea to support constants on `extra_labels`, we have metrics like `apisix_node_info`, maybe we should extend it to support more constants.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1290171274
> Even with extended `apisix_node_info`, how can i query metrics group by any combination of these labels, such as provider=aws, or deploy=vm&type=public ?
Understood, then you need to add test cases and update the documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1305453717
> > That's great.
> > And how to add a test case in apisix? Any example ?
>
> ref: https://github.com/apache/apisix/blob/master/docs/en/latest/plugin-develop.md#write-test-case
Test case & doc are provided. Pls review it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1289230569
Why support const? What scenarios require monitoring a constant value?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1302851927
> That's great.
> And how to add a test case in apisix? Any example ?
ref: https://github.com/apache/apisix/blob/master/docs/en/latest/plugin-develop.md#write-test-case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1305316420
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1022371891
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) then
Review Comment:
@xinxinh2020
This branch will match `a$b`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1019913138
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) then
Review Comment:
not, "a$b" will be considered as a const now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1030108118
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) == 1 then
Review Comment:
Would be better to use string.byte which is O(1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1019913138
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) then
Review Comment:
no, "a$b" will be considered as a const now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1289903912
> Why support const? What scenarios require monitoring a constant value?
In my case, i need to identify which apisix cluster the metrics come from。and every apisix clusters，which are managed by k8s operator, have it own labels(such as which k8s CR it belongs to). I think it would be useful to tag these labels in metrics to make us more easy to query metrics of specific custom labels.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1301733381
>
That's great.
And how to add a test case in apisix? Any example ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1028787538
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) then
Review Comment:
> @xinxinh2020 This branch will match `a$b`.
Thinks for reminding. I fix it already.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1016229678
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) then
Review Comment:
Will "a$b" match?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 closed pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 closed pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
URL: https://github.com/apache/apisix/pull/8159
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by github-actions.
github-actions[bot] commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1401666230
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] xinxinh2020 commented on pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
xinxinh2020 commented on PR #8159:
URL: https://github.com/apache/apisix/pull/8159#issuecomment-1301733788
> > Even with extended `apisix_node_info`, how can i query metrics group by any combination of these labels, such as provider=aws, or deploy=vm&type=public ?
>
> Understood, then you need to add test cases and update the documentation.
That's great.
And how to add a test case in apisix? Any example ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] closed pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
URL: https://github.com/apache/apisix/pull/8159
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #8159: feat(prometheus): extra_labels of prometheus plugin_attr support const
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8159:
URL: https://github.com/apache/apisix/pull/8159#discussion_r1022372512
##########
apisix/plugins/prometheus/exporter.lua:
##########
@@ -80,9 +80,14 @@ local function extra_labels(name, ctx)
for _, kv in ipairs(labels) do
local val, v = next(kv)
if ctx then
- val = ctx.var[v:sub(2)]
- if val == nil then
- val = ""
+ if v:find("$",1, true) then
Review Comment:
```
￥ cat /tmp/x.lua
#!/usr/bin/env luajit
local v = "a$b"
print(v:find("$",1, true))
￥ /tmp/x.lua
2 2
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org