-
Notifications
You must be signed in to change notification settings - Fork 57
fix(exporter) attach subsystem label to memory stats #118
Conversation
1610d21
to
3fdaebb
Compare
Please resolve the conflict and rebase. |
666c899
to
54e87dc
Compare
@hbagdi Could you take another review? |
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.
LGTM other than the two minor comments.
d4b7436
to
e82cf8c
Compare
Need to fix the test. |
Memory stats in HTTP and Stream subsystem are independent, attaching the label so prometheus won't complain about duplicate metrics
df4e64a
to
ac11f75
Compare
ac11f75
to
7393b1d
Compare
7393b1d
to
bc65f93
Compare
The tests are failing but seems not related 🤔 @hbagdi I addressed your comments and if there's no objection from you I'll merge this PR. |
. | ||
# HELP kong_memory_lua_shared_dict_total_bytes Total capacity in bytes of a shared_dict | ||
# TYPE kong_memory_lua_shared_dict_total_bytes gauge | ||
kong_memory_lua_shared_dict_total_bytes{shared_dict="kong",kong_subsystem="http"} 5242880 |
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 metric seems to be different from the rest of the others. All the others are usage metrics but this one seems like a capacity metric.
Should this instead be named as kong_memory_lua_shared_dict_capacity_bytes
?
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.
@hbagdi This is indeed a capacity metric, but it's not newly added in this PR.
I add it into the Readme because it seems got missed before. Consider its
age in the code we should probably keep with the old naming?
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.
Ah, correct. That would be a breaking change.
I was about to approve but had one last comment. |
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'm approving but please take care of the CI soon.
- Fix exporter to attach subsystem label to memory stats [#118](Kong/kong-plugin-prometheus#118) - Expose dataplane status on control plane, new metrics `data_plane_last_seen`, `data_plane_config_hash` and `data_plane_version_compatible` are added. [#98](Kong/kong-plugin-prometheus#98)
- Fix exporter to attach subsystem label to memory stats [#118](Kong/kong-plugin-prometheus#118) - Expose dataplane status on control plane, new metrics `data_plane_last_seen`, `data_plane_config_hash` and `data_plane_version_compatible` are added. [#98](Kong/kong-plugin-prometheus#98)
Memory stats in HTTP and Stream subsystem are independent, attaching the label so prometheus won't complain about duplicate metrics
Memory stats in HTTP and Stream subsystem are independent,
attaching the label so prometheus won't complain about duplicate metrics