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.

Sign up for GitHub

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

Jump to bottom

feat(plugin) Adjust Prometheus to be the reference Impl for metrics #8712

Merged
merged 4 commits into from
Jun 1, 2022
Merged

feat(plugin) Adjust Prometheus to be the reference Impl for metrics #8712

merged 4 commits into from
Jun 1, 2022

Conversation

hfukada
Copy link

@hfukada hfukada commented Apr 21, 2022 •
edited
Loading

Rework and Upgrade Prometheus Plugins Metrics and Labels.

Summary

These Prometheus plugin will be the first class citizen for metrics collection in Kong 3.0 and beyond. This patch attempts to lay foundation for a good set of base metrics+labels, that can also be expanded upon in later releases

Full changelog

  • Latency has been split into 4 different metrics: kong_latency_ms, upstream_latency_ms and request_latency_ms (http) /tcp_session_duration_ms (stream). Buckets details below.
  • Separate out Kong Latency Bucket values and Upstream Latency Bucket values.
    • Kong Latency and Upstream Latency can operate at orders of magnitudes different. Separate out these buckets to reduce memory overhead.
  • consumer_status removed.
  • request_count and consumer_status have been merged into just http_requests_total. If the per_consumer config is set false, the consumer label will be empty. If the per_consumer config is true, it will be filled.
  • http_requests_total has a new label source. set to either exit, error or service. https://docs.konghq.com/gateway/latest/pdk/kong.response/#kongresponseget_source
  • New Metric: node_info. Single gauge set to 1 that outputs the node's id and kong version.
  • All Memory metrics have a new label node_id
  • nginx_http_current_connections merged with nginx_stream_current_connection to nginx_current_connections
  • Plugin Version Bumped to 3.0

@hfukada hfukada requested a review from a team as a code owner April 21, 2022 16:19
@CLAassistant
Copy link

CLAassistant commented Apr 21, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@hbagdi hbagdi changed the title feat(PrometheusPluginRework) Adjust Prometheus to be the reference Impl for metrics feat(plugin) Adjust Prometheus to be the reference Impl for metrics Apr 21, 2022
@flrgh flrgh requested review from hbagdi and casimiro May 2, 2022 19:50
hbagdi
hbagdi reviewed May 3, 2022
View reviewed changes
Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly LGTM. @fffonion Could you do another review as well, please.

kong/plugins/prometheus/exporter.lua Outdated Show resolved Hide resolved
kong/plugins/prometheus/exporter.lua Outdated Show resolved Hide resolved
spec/03-plugins/26-prometheus/02-access_spec.lua Show resolved Hide resolved
@RobSerafini RobSerafini requested a review from hbagdi May 6, 2022 21:10
@hfukada hfukada requested review from fffonion and locao May 9, 2022 03:48
@hfukada hfukada force-pushed the prometheus-3.0-refresh branch 2 times, most recently from f47c76a to f2df285 Compare May 9, 2022 06:53
@fffonion
Copy link
Contributor

fffonion commented May 9, 2022

When we are renaming metrics, should we take this as chance to compliant with Prometheus as suggested by #8237?

@hfukada hfukada force-pushed the prometheus-3.0-refresh branch 2 times, most recently from d3132ff to 885ca1e Compare May 10, 2022 11:28
@hfukada hfukada force-pushed the prometheus-3.0-refresh branch 2 times, most recently from 45a0a87 to b0663b3 Compare May 11, 2022 08:09
fffonion
fffonion reviewed May 11, 2022
View reviewed changes
kong/plugins/prometheus/exporter.lua Outdated Show resolved Hide resolved
kong/plugins/prometheus/exporter.lua Outdated Show resolved Hide resolved
kong/plugins/prometheus/exporter.lua Outdated Show resolved Hide resolved
kong/plugins/prometheus/exporter.lua Show resolved Hide resolved
kong/plugins/prometheus/exporter.lua Show resolved Hide resolved
@hfukada hfukada force-pushed the prometheus-3.0-refresh branch 2 times, most recently from 1282612 to 5061c16 Compare May 11, 2022 10:35
@hfukada hfukada force-pushed the prometheus-3.0-refresh branch 4 times, most recently from 082ec0b to 4309b9f Compare May 11, 2022 23:42
@hfukada hfukada added this to the 3.0 milestone May 12, 2022
@fffonion fffonion mentioned this pull request May 12, 2022
Closed
@fffonion
Copy link
Contributor

Code LGTM, let's make sure this is recorded in changelog (https://github.com/Kong/kong/blob/master/CHANGELOG.md#plugins) especially to mention the breaking changes.

@fffonion fffonion mentioned this pull request May 13, 2022
Closed
hfukada and others added 2 commits May 29, 2022 19:55
@hfukada
feat(PrometheusPluginRework) Prometheus 3.0 Rework
93d3dce 
- Rework and Upgrade Prometheus Plugins Metrics and Labels
- fix(prometheus) move total out of current_connections metrics and
- create new nginx_{http,stream}_total_requests
@casimiro @hfukada
addressing metric naming feedback
4d0b15c 
- metric naming
- metric source naming
@fffonion
Copy link
Contributor

fffonion commented Jun 1, 2022

I fixed an error in the session_latency_ms to remove tcp (as it also covers UDP).

@fffonion fffonion merged commit 2215828 into master Jun 1, 2022
@fffonion fffonion deleted the prometheus-3.0-refresh branch June 1, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbagdi hbagdi hbagdi left review comments

@sichvoge sichvoge sichvoge left review comments

@fffonion fffonion fffonion approved these changes

@casimiro casimiro casimiro approved these changes

@locao locao Awaiting requested review from locao

Assignees
No one assigned
Labels
plugins/prometheus size/L
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
7 participants
@hfukada @CLAassistant @fffonion @casimiro @sichvoge @hbagdi @flrgh