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

feat(prosody): openmetrics module support #1832

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

24kushang
Copy link
Contributor

@24kushang 24kushang commented Jun 24, 2024

Enable Prosody Metrics Module in Jitsi Meet

Summary:
This PR enhances the Jitsi Meet Docker setup by adding support for metrics scraping for the Prosody service. This allows for improved monitoring and observability of the Prosody service within the Jitsi Meet deployment.

Changes:

  1. Prosody Configuration Update:

    • Modified prosody.cfg.lua to enable the http_openmetrics module. This activates the /metrics endpoint to serve metrics.
    • Added statistics provider and interval configuration to generate basic statistics for the Prosody service.
  2. Network Configuration:

    • Updated the configuration to include the CIDR block of the Docker subnet, allowing access to the metrics endpoint within the internal Docker network.
    • Created an environment variable PROSODY_METRICS_ALLOWED_CIDR with default subnet 172.16.0.0/12 which can be changed as per requirement.
  3. Environment Variable:

    • Introduced PROSODY_ENABLE_METRICS environment variable to control the metrics feature. It is set to false by default but can be changed to true to enable metrics collection.

How to Use:

  1. Update Environment Configuration:

    • Add the following environment variable to your .env file:
      PROSODY_ENABLE_METRICS=true
      PROSODY_METRICS_ALLOWED_CIDR=IP_RANGE_OF_YOUR_CHOICE   # optional
  2. Rebuild and Deploy:

    • Rebuild your Docker containers to apply the new configuration:
      docker-compose down
      docker-compose up -d
  3. Verify Metrics Collection:

    • Access the /metrics endpoint of the Prosody service to ensure that metrics are being served. You can verify this by navigating to http://<prosody-service-ip>:5280/metrics in your browser or using a tool like curl.

Notes:

  • Make sure the IP addresses and network configurations match your existing setup.
  • Adjust the statistics_interval as per your monitoring needs.

Contributor:


Copy link
Member

@aaronkvanmeerten aaronkvanmeerten left a comment

Choose a reason for hiding this comment

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

The name of the PR should be something like the below in order to follow the guidelines of the repo:

feat(prosody): openmetrics module support

@@ -28,6 +28,7 @@
{{ $XMPP_GUEST_DOMAIN := .Env.XMPP_GUEST_DOMAIN | default "guest.meet.jitsi" -}}
{{ $XMPP_MUC_DOMAIN := .Env.XMPP_MUC_DOMAIN | default "muc.meet.jitsi" -}}
{{ $XMPP_PORT := .Env.XMPP_PORT | default "5222" -}}
{{ $PROSODY_ENABLE_METRICS := .Env.PROSODY_ENABLE_METRICS | default "false" | toBool -}}
Copy link
Member

Choose a reason for hiding this comment

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

This should be sorted alphabetically with the other variables

prosody/rootfs/defaults/prosody.cfg.lua Outdated Show resolved Hide resolved
{{ if $PROSODY_ENABLE_METRICS }}
statistics = "internal"
statistics_interval = "manual"
openmetrics_allow_cidr = "172.18.0.0/16"
Copy link
Member

Choose a reason for hiding this comment

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

This should be a configurable CIDR via env variable and default to 172.16.0.0/12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a bit confused in cidr block. /12 or /16 should be the default one?

Copy link
Member

Choose a reason for hiding this comment

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

This should be a variable, and the default value can be 172.16.0.0/12 which is the full CIDR for the reserved IP space that docker is default configured to use. The smaller CIDR you list above 172.18.0.0/16 will only work for 172.18.0.0 - 172.18.0.256, but some docker setups use 172.16.x.x or 172.17.x.x by default, and many people override their docker to use other reserved IP spaces such as 192.168.x.x or 10.x.x.x, so it's important to have a sensible default but allow for overrides as needed.

@24kushang 24kushang changed the title Metrics enabled in Prosody Service feat(prosody): openmetrics module support Jun 25, 2024
24kushang and others added 3 commits June 25, 2024 18:46
Changed according to the PR review

- added the PROSODY_METRICS_ALLOWED_CIDR variable
- arranged the variable in alphabetic order
Added 2 environment variable for prosody metrics collection

- PROSODY_ENABLE_METRICS
- PROSODY_METRICS_ALLOWED_CIDR
@24kushang
Copy link
Contributor Author

Updated the changes as per discussion. The PR is ready for review

@@ -12,7 +12,9 @@
{{ $GC_GEN_MAX_TH := .Env.GC_GEN_MAX_TH | default 100 -}}
{{ $LOG_LEVEL := .Env.LOG_LEVEL | default "info" }}
{{ $PROSODY_C2S_LIMIT := .Env.PROSODY_C2S_LIMIT | default "10kb/s" -}}
{{ $PROSODY_METRICS_ALLOWED_CIDR := .Env.PROSODY_METRICS_ALLOWED_CIDR | default "172.16.0.0/12" | toString -}}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{ $PROSODY_METRICS_ALLOWED_CIDR := .Env.PROSODY_METRICS_ALLOWED_CIDR | default "172.16.0.0/12" | toString -}}
{{ $PROSODY_METRICS_ALLOWED_CIDR := .Env.PROSODY_METRICS_ALLOWED_CIDR | default "172.16.0.0/12" -}}

No need for toString here I don't believe, since the default is already a string.

@@ -108,6 +110,12 @@ modules_enabled = {
"s2sout_override";
"s2s_whitelist";
{{ end -}}

-- metrics collection functionality
Copy link
Member

Choose a reason for hiding this comment

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

This should be inside the if statement, as otherwise it will be rendered in the final document no matter whether metrics are enabled or not.

@@ -287,6 +295,13 @@ log = {
{{ end }}
}

-- Statistics Provider and
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this should be inside the if statement

@@ -287,6 +295,13 @@ log = {
{{ end }}
}

-- Statistics Provider and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- Statistics Provider and
-- Statistics Provider configuration

@aaronkvanmeerten aaronkvanmeerten merged commit fbc250e into jitsi:master Jun 26, 2024
1 check passed
saghul added a commit that referenced this pull request Jul 2, 2024
* b4604f3 feat(whiteboard) add builtin whiteboard integration
* 49bd165 feat(jibri) update Chrome to 126
* 980703b fix(jibri): display modes in xorg config
* c65d2bd feat(jigasi,jicofo,jvb,jibri,prosody): add healthcheck.sh scripts and prosody http_health mod
* d2ac43d feat(jigasi): Add new environment variables for jigasi configuration (#1841)
* fbc250e feat(prosody): openmetrics module support (#1832)
* 2adac72 feat(prosody): Drop non existing config.
* e22b4f3 feat(jibri): Adds more fonts.
* 982e577 feat(jicofo,jvb,web) default to using SCTP based datachannels
* 55c975f fix(jicofo): use integer for port value to fix addition (#1826)
* 7c8763f base: update tpl to version 1.3.0
* 1d3c0f1 jibri: fail to start if the SYS_ADMIN cap is missing (#1816)
* cacee4e jvb: adds autoscaler sidecar hook to signal final shutdown (#1809)
* 726b0f0 jibri: adds autoscaler sidecar hook to signal final shutdown (#1810)
* f2b2cc4 etherpad: update image version and settings
* 731d6d1 base: update tpl to make toBool more resilient
* 59c4eb2 jvb: add fallback WS server ID
* 9a54843 jvb: add ability to configure the TLS option for colibri WS
* 79a4635 jvb: add ability to disable XMPP
* 77ce86a jvb: add ability to enable SCTP datachannels
* 76ffaa7 jicofo, jvb: fix OCTO + SCTP behaviour
* 0298a30 misc: working on unstable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants