Clarify use of 'FCM' in app type and refactor to use 'FCM' #251

H-Shay · 2021-08-20T20:05:05Z

Addresses issue #230. This PR allows for both gcm and fcm to be used as app types, warns that the app type gcm will soon be depreciated, and refactors sygnal to clarify that we are using Firebase Cloud Messaging exclusively.

Signed-off-by: H.Shay <shaysquared@gmail.com>

H-Shay · 2021-08-20T20:33:35Z

Totally baffled as to why the changelog check is failing.

anoadragon453

Totally baffled as to why the changelog check is failing.

Looks like the CI is finding the changelog at <checkout dir>/sygnal/changelog.d/251.misc, rather than <checkout dir>/changelog.d/251.misc, which its expecting.

The changelog CI code was just recently updated, and the changelog location in your PR looks correct. I might suggest trying to merge in the main branch and see if that works.

README.rst

changelog.d/251.misc

sygnal.yaml.sample

anoadragon453 · 2021-08-23T13:05:32Z

sygnal/fcmpushkin.py

-    "sygnal_gcm_queue_time", "Time taken waiting for a connection to GCM"
+    "sygnal_fcm_queue_time", "Time taken waiting for a connection to FCM"
 )

 SEND_TIME_HISTOGRAM = Histogram(
-    "sygnal_gcm_request_time", "Time taken to send HTTP request to GCM"
+    "sygnal_fcm_request_time", "Time taken to send HTTP request to FCM"
 )

 PENDING_REQUESTS_GAUGE = Gauge(
-    "sygnal_pending_gcm_requests", "Number of GCM requests waiting for a connection"
+    "sygnal_pending_fcm_requests", "Number of FCM requests waiting for a connection"
 )

 ACTIVE_REQUESTS_GAUGE = Gauge(
-    "sygnal_active_gcm_requests", "Number of GCM requests in flight"
+    "sygnal_active_fcm_requests", "Number of FCM requests in flight"
 )

 RESPONSE_STATUS_CODES_COUNTER = Counter(
-    "sygnal_gcm_status_codes",
-    "Number of HTTP response status codes received from GCM",
+    "sygnal_fcm_status_codes",
+    "Number of HTTP response status codes received from FCM",
    labelnames=["pushkin", "code"],


Note that modifying these will likely break existing metrics dashboards. Thus this (as well as the span tag name changes below) will need to be detailed in the changelog as well so sysadmins aren't caught off guard 🙂

So afaict my main branch is up to date with the latest sygnal changes, and this fork as well-attempting to merge from main just gives an "Already up to date" message, yet the changelog check is still failing. I can't tell if I am missing something simple here!

Looks like it's passing now! (Thanks @reivilibre!)

reivilibre · 2021-08-23T18:21:49Z

the changelog issue is odd. It was shown to work in the PR that added it :|. I can't see what's wrong with it or why it would act differently in either case.

reivilibre · 2021-08-23T18:30:35Z

it seems like there's an issue with the newsfile checker when used with a fork rather than a branch, as I found out by experimenting with #253 (and #252).
I'll look into this, sorry for the hassle (edit: #254 should address this)

anoadragon453

Looks good to me other than the minor bits below!

anoadragon453 · 2021-08-24T16:45:16Z

sygnal/fcmpushkin.py

-    "sygnal_gcm_queue_time", "Time taken waiting for a connection to GCM"
+    "sygnal_fcm_queue_time", "Time taken waiting for a connection to FCM"
 )

 SEND_TIME_HISTOGRAM = Histogram(
-    "sygnal_gcm_request_time", "Time taken to send HTTP request to GCM"
+    "sygnal_fcm_request_time", "Time taken to send HTTP request to FCM"
 )

 PENDING_REQUESTS_GAUGE = Gauge(
-    "sygnal_pending_gcm_requests", "Number of GCM requests waiting for a connection"
+    "sygnal_pending_fcm_requests", "Number of FCM requests waiting for a connection"
 )

 ACTIVE_REQUESTS_GAUGE = Gauge(
-    "sygnal_active_gcm_requests", "Number of GCM requests in flight"
+    "sygnal_active_fcm_requests", "Number of FCM requests in flight"
 )

 RESPONSE_STATUS_CODES_COUNTER = Counter(
-    "sygnal_gcm_status_codes",
-    "Number of HTTP response status codes received from GCM",
+    "sygnal_fcm_status_codes",
+    "Number of HTTP response status codes received from FCM",
    labelnames=["pushkin", "code"],


Looks like it's passing now! (Thanks @reivilibre!)

anoadragon453 · 2021-08-24T16:46:08Z

changelog.d/251.misc

@@ -0,0 +1,15 @@
+Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the future deprcation of gcm as an app type. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:


Suggested change

Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the future deprcation of gcm as an app type. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:

Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the future deprecation of gcm as an app type. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:

Additionally, there's still a few places which say "will deprecate" rather than "is deprecated" which should be updated 🙂

anoadragon453 · 2021-08-24T16:47:26Z

changelog.d/251.misc

+    gcm_num_devices -> fcm_num_devices
+    gcm_num_failed -> fcm_num_failed
+
+If you make use of either Prometheus (and thus Grafana, etc) metrics or use OpenTracing with Sygnal, please ensure your queries are up to date.


Thanks for including this information! Note that newsfragments (this file) should just be one line that gives a brief summary. For the call out of the deprecation, that should just be placed at the top of the CHANGELOG.md file as part of this PR (rather than the newsfragment).

reivilibre

some little tweaks I've spotted. I think anoa will be in charge of giving this the tick though still?

reivilibre · 2021-08-24T17:51:08Z

CHANGELOG.md

@@ -1,3 +1,19 @@
+This release deprecates the use of gcm as an app type and refactors Sygnal to use fcm as an app type exclusively. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:


Suggested change

This release deprecates the use of gcm as an app type and refactors Sygnal to use fcm as an app type exclusively. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:

This release deprecates the use of `gcm` as an app type and refactors Sygnal to use `fcm` as an app type exclusively. Due to these changes, some Prometheus metrics' names were updated to refer to Firebase Cloud Messaging (FCM) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:

reivilibre · 2021-08-24T17:54:52Z

changelog.d/251.misc

@@ -0,0 +1 @@
+Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the deprecation of gcm as an app type.


Suggested change

Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the deprecation of gcm as an app type.

Rename the `gcm` app type to `fcm` to reduce confusion (Google Cloud Messaging changed name to Firebase Cloud Messaging in 2016). The app type `gcm` is deprecated and will be removed in a future release.

reivilibre · 2021-08-24T17:56:42Z

sygnal.yaml.sample

  #
  #com.example.myapp.android:
-  #  type: gcm
-  #  api_key: your_api_key_for_gcm
+  #  type: fcm or gcm


Suggested change

# type: fcm or gcm

# type: fcm

no point suggesting something that's deprecated, I'd say (also it should be directly uncommentable so that it works)

reivilibre · 2021-08-24T18:03:36Z

sygnal.yaml.sample

+  # This is an example FCM/GCM push configuration. Note that the gcm type is a legacy
+  # setting as we now use Firebase Cloud Messaging (fcm) exclusively. Setting gcm as the
+  # app type is still accepted but understand that under the hood we are using fcm.
+  # The gcm app type is deprecated.


Suggested change

# This is an example FCM/GCM push configuration. Note that the gcm type is a legacy

# setting as we now use Firebase Cloud Messaging (fcm) exclusively. Setting gcm as the

# app type is still accepted but understand that under the hood we are using fcm.

# The gcm app type is deprecated.

# This is an example FCM push configuration.

# FCM was formerly known as GCM.

# The deprecated app type `gcm` is equivalent but will be removed from a future Sygnal release.

reivilibre · 2021-08-24T18:05:58Z

sygnal/fcmpushkin.py

 # We include NotRegistered here too for good measure, even
-# though gcm-client 'helpfully' extracts these into a separate
+# though fcm-client 'helpfully' extracts these into a separate
 # list.
 BAD_PUSHKEY_FAILURE_CODES = [


Suggested change

# We include NotRegistered here too for good measure, even

# though gcm-client 'helpfully' extracts these into a separate

# though fcm-client 'helpfully' extracts these into a separate

# list.

BAD_PUSHKEY_FAILURE_CODES = [

BAD_PUSHKEY_FAILURE_CODES = [

gcm-client was a library for using GCM, so renaming the reference to fcm-client doesn't make sense anymore.

However, gcm-client isn't even in use, so this comment is probably very crufty — hence I think it should just be removed altogether.

reivilibre

maybe anoa will have opinions about ' vs `.

CHANGELOG.md

changelog.d/251.misc

callahad · 2021-08-25T08:49:38Z

Without looking at this too closely, do we have opinions about referring to this as "FCM" versus something more specific to the FCM API we implement?

Namely, Sygnal supports the "legacy HTTP" API, not the newer "HTTP v1" API (cite: Migrate from legacy HTTP to HTTP v1)

Concretely this would probably mean using fcm_legacy or similar instead of just fcm so that we have room for fcm_v1 later.

reivilibre · 2021-08-25T09:07:30Z

Without looking at this too closely, do we have opinions about referring to this as "FCM" versus something more specific to the FCM API we implement?

Namely, Sygnal supports the "legacy HTTP" API, not the newer "HTTP v1" API (cite: Migrate from legacy HTTP to HTTP v1)

Concretely this would probably mean using fcm_legacy or similar instead of just fcm so that we have room for fcm_v1 later.

On one hand, I don't see much advantage, unless we intend to support both separately? They may be two different APIs, but they are both APIs onto the same service. We never had an app type change when we moved over from the old, binary-protocol APNS to the HTTP/2 APNS, for example. Renaming it to something with 'legacy' in the name is kind of unfortunate when it's the 'gcm' option that's the legacy option.

On the other hand, perhaps we will migrate to the v1 API one day (if there's a use case for it). I can't tell if admins will need to make any config changes then or not (it seems like the format of access tokens might have changed, so perhaps?). If that's the case, I can feel someone wanting to say that we should honour both options (:P) for some time whilst admins migrate (although I suppose nobody is forcing them to upgrade Sygnal until they're ready to amend the configuration).

anoadragon453 · 2021-08-25T14:22:36Z

Having fcm_legacy without a fcm sounds a bit confusing. I'd suggest we either just do fcm_v1 when the time comes, or simply add an additional option under the pushkin config to choose the API, while still keeping an app type of fcm.

anoadragon453

I have no more suggestions other than to resolve the open discussions above 🙂

rltas · 2023-02-16T09:12:32Z

What about supporting both type: gcm and type: fcm and introducing a new parameter server_key for the latter?
Then for a transition period, existing configs keep working with the legacy API without config changes:
type: gcm
api_key: your_api_key_for_gcm
But if you're going with fcm, it would have to be:
type: fcm
server_key: your_server_key_for_gcm
This would be more proper (less confusing) naming anyway.
Then when #313 is ready, issue deprecation warnings on startup for a transition period when server_key is used, before finally removing the old code altogether, because Google will remove the legacy API at some point.
This would be a good process for those who have to maintain Sygnal (admins), because they're usually not the ones doing the Google/app stuff.
Because this change has been delayed for quite some time, while Matrix gained quite a bit of popularity, many new setups unfortunately now use type: gcm and have to be given some head notice.
When using the ansible playbook to setup a Matrix stack, this can be elegantly mitigated anyway.

H-Shay added 8 commits August 20, 2021 12:43

update docs and rename files

5341baf

refactor files to use fcm instead of gcm

3add50f

add changelog

e084e08

Signed-off-by: H.Shay <shaysquared@gmail.com>

update changelog number

73654cd

update changelog

cc0a87b

test

0370eaf

test

bcdb295

fix changelog

e068280

clokep requested a review from a team August 23, 2021 12:13

anoadragon453 requested changes Aug 23, 2021

View reviewed changes

fix typos, update changelog

819c065

H-Shay requested a review from anoadragon453 August 23, 2021 17:37

Merge branch 'main' into update_gcm_fcm

5751c57

anoadragon453 requested changes Aug 24, 2021

View reviewed changes

update to be clear about gcm deprecation

58c5238

H-Shay requested a review from anoadragon453 August 24, 2021 17:35

reivilibre reviewed Aug 24, 2021

View reviewed changes

requested changes

276e448

reivilibre reviewed Aug 25, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

changelog.d/251.misc Show resolved Hide resolved

anoadragon453 reviewed Aug 25, 2021

View reviewed changes

H-Shay closed this Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify use of 'FCM' in app type and refactor to use 'FCM' #251

Clarify use of 'FCM' in app type and refactor to use 'FCM' #251

H-Shay commented Aug 20, 2021

H-Shay commented Aug 20, 2021

anoadragon453 left a comment

anoadragon453 Aug 23, 2021

H-Shay Aug 23, 2021

anoadragon453 Aug 24, 2021

reivilibre commented Aug 23, 2021

reivilibre commented Aug 23, 2021 •

edited

Loading

anoadragon453 left a comment

anoadragon453 Aug 24, 2021

anoadragon453 Aug 24, 2021

anoadragon453 Aug 24, 2021

reivilibre left a comment

reivilibre Aug 24, 2021

reivilibre Aug 24, 2021

reivilibre Aug 24, 2021

reivilibre Aug 24, 2021

reivilibre Aug 24, 2021

reivilibre left a comment

callahad commented Aug 25, 2021

reivilibre commented Aug 25, 2021

anoadragon453 commented Aug 25, 2021

anoadragon453 left a comment

rltas commented Feb 16, 2023

		@@ -0,0 +1,15 @@
		Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the future deprcation of gcm as an app type. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:

		@@ -1,3 +1,19 @@
		This release deprecates the use of gcm as an app type and refactors Sygnal to use fcm as an app type exclusively. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:

	This release deprecates the use of gcm as an app type and refactors Sygnal to use fcm as an app type exclusively. Due to these changes, some prometheus metrics names were updated to refer to Firebase Cloud Messaging (fcm) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:
	This release deprecates the use of `gcm` as an app type and refactors Sygnal to use `fcm` as an app type exclusively. Due to these changes, some Prometheus metrics' names were updated to refer to Firebase Cloud Messaging (FCM) instead of the legacy Google Cloud Messaging (GCM) services. You can find a list of the changed metric names below:

		@@ -0,0 +1 @@
		Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the deprecation of gcm as an app type.

	Addresses #230, refactors sygnal to use fcm as app type rather than gcm, and warns in documents about the deprecation of gcm as an app type.
	Rename the `gcm` app type to `fcm` to reduce confusion (Google Cloud Messaging changed name to Firebase Cloud Messaging in 2016). The app type `gcm` is deprecated and will be removed in a future release.

Clarify use of 'FCM' in app type and refactor to use 'FCM' #251

Clarify use of 'FCM' in app type and refactor to use 'FCM' #251

Conversation

H-Shay commented Aug 20, 2021

H-Shay commented Aug 20, 2021

anoadragon453 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reivilibre commented Aug 23, 2021

reivilibre commented Aug 23, 2021 • edited Loading

anoadragon453 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reivilibre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reivilibre left a comment

Choose a reason for hiding this comment

callahad commented Aug 25, 2021

reivilibre commented Aug 25, 2021

anoadragon453 commented Aug 25, 2021

anoadragon453 left a comment

Choose a reason for hiding this comment

rltas commented Feb 16, 2023

reivilibre commented Aug 23, 2021 •

edited

Loading