Store server's version number, include in Sentry events #3745

gnprice · 2019-12-31T05:56:42Z

When an error happens only on certain versions of the Zulip server, that can be a powerful clue for diagnosing it. I've often noticed when looking at Sentry events that I'd like to have that information. #3732 is a recent example that brought this to mind.

In implementing this, one layer is to use the Sentry API to get that information attached. I'm not sure how best to do that; @ray-kraesig has studied those docs recently and may have ideas.

The other layer is to get and keep the server-version information around in the first place. The server reports its version number in the server_settings response, but we don't currently store it anywhere; the only things we use the server_settings response for are at initial login, so we fetch it once then. It wouldn't do to just store it then, either, because of course the version (and other parts of that response) can change over the months and years the user might stay logged in.

Here's a plausible approach:

Add a property like serverVersion to the Account type, found in the accounts state-subtree. Can be null | string.
In the login flow, save the version we get.
At the same time we call /register aka registerForEvents, in doInitialFetch in fetchActions.js, make a /server_settings request again and save the new version value.

The server version number could also be useful if we decide to condition on it for certain API changes, as suggested at #3732 (comment) .

The text was updated successfully, but these errors were encountered:

rk-for-zulip · 2019-12-31T20:55:45Z

In implementing this, one layer is to use the Sentry API to get that information attached. I'm not sure how best to do that; @ray-kraesig has studied those docs recently and may have ideas.

Sounds like a job for either tags or extras. We probably want something like this when handling the server_settings response:

Sentry.configureScope(scope => { scope.setTag("server-version", "2.2.7723"); });

Note that I haven't tried tags out yet, so I'm not sure what they look like in Sentry – but extras are used in #3733.

gnprice · 2020-01-01T01:29:52Z

Sounds like a job for either tags or extras.

Thanks!

Between those two, I agree with your example that we probably want tags. Seems like the main difference is that tags get indexed, searched by, and aggregated on -- all of which sound like useful things for the server version.

Thinking about the search and aggregation, one thing that occurs to me is we likely want several tags here:

the full version string, for complete information (e.g. 2.1.0-66-gd8a2ec0, the current value of curl -s https://python.zulipchat.com/api/v1/server_settings | jq .zulip_version -r)
one or two simplified version strings, making it more coarse-grained for aggregation. (e.g. 2.1, or for explicitness perhaps 2.1.x)

rk-for-zulip · 2020-01-02T18:24:05Z

One reason I'm not sure we want tags is because they may also get disaggregated on – that is, a truly client-side-only issue might end up separated into fifty different issues just because the clients exhibiting it are connected to fifty different server versions.

(The links at the top of this documentation describe ways to customize the grouping, though, so even if that does happen we can probably override it.)

gnprice · 2020-01-03T02:51:44Z

Mmm, I see. I think that's not what will happen because the examples include:

The version of your platform (e.g. iOS 5.0)

and so I expect it to behave like the existing fields of that flavor do: they don't affect how the events get grouped into issues, and instead they get counted up across all the events of an issue to produce the graphs seen at the RHS of an event page.

But yeah, if we get surprised in that respect then we'll figure out a way to deal. 🙂

chrisbobbe · 2020-01-23T20:14:56Z

I can make a PR with the following —

Add a property like serverVersion to the Account type, found in the accounts state-subtree. Can be null | string.

In the login flow, save the version we get.

At the same time we call /register aka registerForEvents, in doInitialFetch in fetchActions.js, make a /server_settings request again and save the new version value.

— and then the other things that depend on the server version (Sentry logging, etc.) can follow, right?

How's this for a workflow: I'll assign this issue to me, submit the PR without a Fixes: mark, and un-assign this issue so the rest of the stuff can be done? (I'm happy to do the rest but it sounds like we have more discussion to do about Sentry, etc.)

First, is there a reason not to just store the entire object received from server_settings?

chrisbobbe · 2020-01-23T22:15:02Z

It seems like the server_settings data (whether just the server version or the whole object) would make sense if made part of both the REALM_ADD and REALM_INIT actions; does this seem right conceptually? The alternative is most likely to create a new action.

chrisbobbe · 2020-01-24T00:20:43Z

OK, so from our chat earlier:

First, is there a reason not to just store the entire object received from server_settings?

Yes, we don't need all that stuff. Just the version number is fine.

It seems like the server_settings data (whether just the server version or the whole object) would make sense if made part of both the REALM_ADD and REALM_INIT actions; does this seem right conceptually? The alternative is most likely to create a new action.

Right, those seem to make sense.

rk-for-zulip · 2020-01-24T00:42:29Z

OK, so from our chat earlier:

First, is there a reason not to just store the entire object received from server_settings?

Yes, we don't need all that stuff. Just the version number is fine.

Maybe not all of it, but we do want some of them in support of other issues. realm_icon is needed for #392, and realm_uri is needed to properly fix... er, the issue that #3671 is intended for.

gnprice · 2020-01-24T01:39:14Z

Maybe not all of it, but we do want some of them in support of other issues. realm_icon is needed for #392, and realm_uri is needed to properly fix... er, the issue that #3671 is intended for.

True! I think we don't want to keep the bulk of it, though (which stops being relevant after we log in.) So I think it'll make most sense to pick out the bits we want selectively, as we add code that wants to use them.

chrisbobbe · 2020-01-24T01:40:52Z

I think we don't want to keep the bulk of it, though

This risks becoming a tangent, but we do at least pass the entire object in dispatch(navigateToAuth(serverSettings)); (RealmScreen.js)

chrisbobbe · 2020-01-24T01:51:32Z

This risks becoming a tangent, but we do at least pass the entire object in dispatch(navigateToAuth(serverSettings)); (RealmScreen.js)

(As noted in-person, this is a one-off use of it and not really a concern.)

gnprice · 2020-02-03T19:53:09Z

Bumping priority because this is on the path to resolving #3732.

gnprice · 2020-03-03T22:38:06Z

#3839 is merged, so we are now storing -- and parsing -- the server version!

gnprice · 2020-04-15T18:59:54Z

We've resolved #3732 thanks to using this information. But I think the remaining part is also a priority: when looking at Sentry issues, I'd really like to be able to see what Zulip server versions were involved.

(Probably I should have filed this as two separate issues in the first place.)

I think the next steps are to add some Sentry tags, in the way Ray describes in the first comment. Specifically, as I added after that:

Thinking about the search and aggregation, one thing that occurs to me is we likely want several tags here:

the full version string, for complete information (e.g. 2.1.0-66-gd8a2ec0, the current value of curl -s https://python.zulipchat.com/api/v1/server_settings | jq .zulip_version -r)

one or two simplified version strings, making it more coarse-grained for aggregation. (e.g. 2.1, or for explicitness perhaps 2.1.x)

chrisbobbe · 2020-04-15T19:03:16Z

#3952 is on the path to this, I think, right? I believe I've addressed the most recent review, but I think there was another strategy we were considering as an alternative to the strategy used in that PR.

rk-for-zulip · 2020-04-16T00:48:05Z

A new selector getServerVersion was added in #4022 (commit 11cca9b). I think that's enough to keep #3952 from blocking this.

gnprice · 2020-04-16T01:19:40Z

Ah, I'd forgotten about that PR, sorry. Yeah, I think it's orthogonal to starting to send the versions in Sentry events.

gnprice · 2020-07-09T03:23:49Z

Bump -- when discussing #4156 we were reminded that this would be a good thing to finish.

Eventually (likely not in this series), we'll add an import of src/boot/store.js from src/utils/logging.js, so we can send the server version to Sentry, for zulip#3745. As Greg demonstrates with a handy tool he made to topologically sort the import graph, that particular import adds a lot of require cycles: enough to make Flow balk and produce over a megabyte of (clearly wrong) error output! This commit won't prevent all of those new cycles, but enough to make the Flow errors go away. We'd like to keep `ApiResponseServerSettings` (and some aliases it uses) alongside the code that actually makes the `server_settings` request; it would be a shame to have to make a separation there (e.g., by putting the types in src/api/apiTypes.js) just because of this Flow output. But, among similar pairs (network-request code with types that describe the response), this is the only one where we import and re-export the types through src/api/apiTypes.js. It's not too awkward just to import directly from the getServerSettings file, and delete the `export type * from ...` line in src/api/apiTypesjs. So, do that. See discussion [1]. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3745.20Server.20version.20to.20Sentry/near/930324

Eventually, we'd like to add an import of src/boot/store.js from src/utils/logging.js, so we can send the server version to Sentry, for zulip#3745. As Greg demonstrates with a handy tool he made to topologically sort the import graph, that particular import adds a lot of require cycles: enough to make Flow balk and produce over a megabyte of (clearly wrong) error output! This commit won't prevent all of those new cycles, but enough to make the Flow errors go away. We'd like to keep `ApiResponseServerSettings` (and some aliases it uses) alongside the code that actually makes the `server_settings` request; it would be a shame to have to make a separation there (e.g., by putting the types in src/api/apiTypes.js) just because of this Flow output. But, among similar pairs (network-request code with types that describe the response), this is the only one where we import and re-export the types through src/api/apiTypes.js. It's not too awkward just to import directly from the getServerSettings file, and delete the `export type * from ...` line in src/api/apiTypesjs. So, do that. See discussion [1]. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3745.20Server.20version.20to.20Sentry/near/930324

gnprice · 2020-11-03T01:13:38Z

We discussed this in chat back in July, and found some obstacles from the fact that trying to get at our Redux store from the logging module introduces a large tangle of import cycles.

That tangle may get resolved when we complete #3804, removing react-navigation from our Redux state, which is looking soon.

If not, a direct workaround would be to break some of the abstraction layers involved: have a tiny module that imports nothing and just exports a global, and have our reducer update that global any time it's updating the server version in our Redux state.

chrisbobbe · 2020-11-04T22:10:43Z

Now that #3804 is resolved, #4191 should bring us within reach of being able to send the server version to Sentry—I just attempted it, and I didn't get any complaints from Flow or Jest! 🙂

Eventually, we'd like to add an import of src/boot/store.js from src/utils/logging.js, so we can send the server version to Sentry, for zulip#3745. As Greg demonstrates with a handy tool he made to topologically sort the import graph, that particular import adds a lot of require cycles: enough to make Flow balk and produce over a megabyte of (clearly wrong) error output! This commit won't prevent all of those new cycles, but enough to make the Flow errors go away. We'd like to keep `ApiResponseServerSettings` (and some aliases it uses) alongside the code that actually makes the `server_settings` request; it would be a shame to have to make a separation there (e.g., by putting the types in src/api/apiTypes.js) just because of this Flow output. But, among similar pairs (network-request code with types that describe the response), this is the only one where we import and re-export the types through src/api/apiTypes.js. It's not too awkward just to import directly from the getServerSettings file, and delete the `export type * from ...` line in src/api/apiTypesjs. So, do that. See discussion [1]. [1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3745.20Server.20version.20to.20Sentry/near/930324

Soon, we'd like to add an import of src/boot/store.js from src/utils/logging.js, so we can send the server version to Sentry, for zulip#3745. As Greg demonstrates with a handy tool he made to topologically sort the import graph, that particular import would make an existing cycle-blob much larger (the technical term is "strongly-connected component" or SCC) if nothing were done about it. So, do something about it -- empirically, this commit prevents that growth [1]. Without doing this, and with that import in place, Flow seems to have a bug: it produces over a megabyte of clearly wrong error output. We'd like to keep `ApiResponseServerSettings` (and some aliases it uses) alongside the code that actually makes the `server_settings` request [2]; it would be a shame to have to make a separation there (e.g., by putting the types in src/api/apiTypes.js) just because of this Flow output. But, among similar pairs (network-request code with types that describe the response), this is the only one where we import and re-export the types through src/api/apiTypes.js. It's not too awkward just to import directly from the getServerSettings file, and delete the `export type * from ...` line in src/api/apiTypesjs. So, do that. [1] See Greg's report of the improvement at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/import.20cycles/near/1057141. [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3745.20Server.20version.20to.20Sentry/near/930324

We include a few tags here: - `rawServerVersion`: Just what it sounds like. - `coarseServerVersion`: E.g., 2.1.x or 3.x - `fineServerVersion`: E.g., 2.1.0 or 3.2 (Note that our numbering conventions changed, effective with 3.0, so 3.x and 4.x are each the same level of granularity as 2.1.x or 2.0.x.) Also, update our custom libdef to include `scope.setTags`, which we use here. Fixes: zulip#3745

gnprice added the a-tools label Dec 31, 2019

gnprice mentioned this issue Dec 31, 2019

Typing notifications are mistyped, resulting in rejection (for servers < 2.0) #3732

Closed

chrisbobbe self-assigned this Jan 23, 2020

chrisbobbe mentioned this issue Jan 24, 2020

Store server version in Redux (WIP for #3745) #3839

Merged

chrisbobbe added the a-redux label Feb 1, 2020

gnprice added the P1 high-priority label Feb 3, 2020

gnprice mentioned this issue Mar 4, 2020

Keep the parsed ZulipVersion in Redux state, rather than raw string #3951

Closed

gnprice mentioned this issue Apr 16, 2020

[WIP] redux: Add transform to keep parsed ZulipVersion in Redux state. #3952

Closed

chrisbobbe mentioned this issue Dec 3, 2020

logging: Send server version to Sentry. #4326

Merged

chrisbobbe closed this as completed in 028ce01 Dec 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store server's version number, include in Sentry events #3745

Store server's version number, include in Sentry events #3745

gnprice commented Dec 31, 2019

rk-for-zulip commented Dec 31, 2019 •

edited

Loading

gnprice commented Jan 1, 2020

rk-for-zulip commented Jan 2, 2020

gnprice commented Jan 3, 2020

chrisbobbe commented Jan 23, 2020 •

edited

Loading

chrisbobbe commented Jan 23, 2020

chrisbobbe commented Jan 24, 2020

rk-for-zulip commented Jan 24, 2020

gnprice commented Jan 24, 2020

chrisbobbe commented Jan 24, 2020

chrisbobbe commented Jan 24, 2020

gnprice commented Feb 3, 2020

gnprice commented Mar 3, 2020

gnprice commented Apr 15, 2020

chrisbobbe commented Apr 15, 2020

rk-for-zulip commented Apr 16, 2020

gnprice commented Apr 16, 2020

gnprice commented Jul 9, 2020

gnprice commented Nov 3, 2020

chrisbobbe commented Nov 4, 2020

Store server's version number, include in Sentry events #3745

Store server's version number, include in Sentry events #3745

Comments

gnprice commented Dec 31, 2019

rk-for-zulip commented Dec 31, 2019 • edited Loading

gnprice commented Jan 1, 2020

rk-for-zulip commented Jan 2, 2020

gnprice commented Jan 3, 2020

chrisbobbe commented Jan 23, 2020 • edited Loading

chrisbobbe commented Jan 23, 2020

chrisbobbe commented Jan 24, 2020

rk-for-zulip commented Jan 24, 2020

gnprice commented Jan 24, 2020

chrisbobbe commented Jan 24, 2020

chrisbobbe commented Jan 24, 2020

gnprice commented Feb 3, 2020

gnprice commented Mar 3, 2020

gnprice commented Apr 15, 2020

chrisbobbe commented Apr 15, 2020

rk-for-zulip commented Apr 16, 2020

gnprice commented Apr 16, 2020

gnprice commented Jul 9, 2020

gnprice commented Nov 3, 2020

chrisbobbe commented Nov 4, 2020

rk-for-zulip commented Dec 31, 2019 •

edited

Loading

chrisbobbe commented Jan 23, 2020 •

edited

Loading