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

feat: make prometheus shared dict configurable #793

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: make prometheus shared dict configurable #793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1474693
feat: make prometheus shared dict configurable
acuteaura Dec 4, 2024
76f84e5
docs: generate helm-docs
acuteaura Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions charts/apisix/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ The command removes all the Kubernetes components associated with the chart and
| apisix.nginx.logs.enableAccessLog | bool | `true` | Enable access log or not, default true |
| apisix.nginx.logs.errorLog | string | `"/dev/stderr"` | Error log path |
| apisix.nginx.logs.errorLogLevel | string | `"warn"` | Error log level |
| apisix.nginx.prometheusSharedDictSize | string | `"15m"` | Set the size of prometheus-metrics shared dictionary |
| apisix.nginx.workerConnections | string | `"10620"` | |
| apisix.nginx.workerProcesses | string | `"auto"` | |
| apisix.nginx.workerRlimitNofile | string | `"20480"` | |
Expand Down
5 changes: 5 additions & 0 deletions charts/apisix/templates/configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,11 @@ data:
{{- if .Values.apisix.nginx.configurationSnippet.stream }}
stream_configuration_snippet: {{- toYaml .Values.apisix.nginx.configurationSnippet.stream | indent 6 }}
{{- end }}
{{- with .Values.apisix.nginx.prometheusSharedDictSize }}
meta:
lua_shared_dict:
prometheus-metrics: {{ . }}
{{- end }}

{{- if .Values.apisix.discovery.enabled }}
discovery:
Expand Down
3 changes: 3 additions & 0 deletions charts/apisix/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,9 @@ apisix:
# - name: bar
# size: 1m

# -- Set the size of prometheus-metrics shared dictionary
prometheusSharedDictSize: 15m
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Is it a good idea to use a separate orphaned key?

Wouldn't it be better to implement it as a map and allow the original shared dict name used by APISIX to be passed in directly for overriding. e.g.

luaSharedDicts:
  - name: prometheus-metrics
    size: 100m

If you think this is unclear, perhaps it could also be called builtinLuaSharedDicts. Or, make it configurable by customLuaSharedDicts, although not intuitive.

BTW, if you update the values template, please update the documentation along with it.

Copy link
Author

@acuteaura acuteaura Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it is very specific (in the otherwise unused meta key) in the upstream config, and the same mechanism doesn't seem to be used anywhere else, so I opted for a single specific key, but feel free to request a specific change if you have something in mind


discovery:
# -- Enable or disable Apache APISIX integration service discovery
enabled: false
Expand Down