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

Usage metrics for v2023.4 #964

Merged
merged 5 commits into from
Sep 12, 2023
Merged

Usage metrics for v2023.4 #964

merged 5 commits into from
Sep 12, 2023

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Aug 29, 2023

Closes #

Here's an xlsx file representing the updated form: https://docs.google.com/spreadsheets/d/1L6GwYeTZjNSilI-pSLRA1ZnoFgfRYLBs/edit#gid=1058536497

Adds 7 new properties under System:

  • sso enabled (still TODO)
  • number of client audit attachments
  • number of client audit attachments that failed processing
  • number of extracted client audit rows
  • number of overall audit log events failed at least once
  • number of overall audit log events failed completely (5 times)
  • number of overall audit log events that were never processed and are over a day old

Versioning of the form changed to match release, e.g. v2023.4.0_1

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.

Before submitting this PR, please make sure you have:

  • run make test-full and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

@@ -30,7 +30,7 @@
"analytics": {
"url": "https://data.getodk.cloud/v1/key/eOZ7S4bzyUW!g1PF6dIXsnSqktRuewzLTpmc6ipBtRq$LDfIMTUKswCexvE0UwJ9/projects/1/submission",
"formId": "odk-analytics",
"version": "2023.05.22.01"
"version": "2023.08.28.01"
Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts about moving to a different versioning pattern? The current pattern guarantees uniqueness, but it requires tracking which form version correlates to which release. It'd be easier if the release were embedded in the form version. What do you think about a form version pattern along the lines of [release]_[form update count within release]. Since this is the first time we're updating the form for v2023.4.0, it would be v2023.4.0_1. That way it'll be easy to see which release the form version corresponds to, yet if for some reason we update the form twice while working on a single release, we'll still have a unique identifier.

@@ -600,6 +605,9 @@ const previewMetrics = () => (({ Analytics }) => Promise.all([

metrics.system.uses_external_db = Analytics.databaseExternal(config.get('default.database.host'));

// TODO for 2023.4
// metrics.system.sso_enabled = (something)
Copy link
Member

Choose a reason for hiding this comment

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

Probably this will entail calling oidc.isEnabled() and converting that boolean to 1/0. There's no database query involved. I don't think this value needs to be rigorously tested.

lib/data/analytics.js Outdated Show resolved Hide resolved
lib/data/analytics.js Outdated Show resolved Hide resolved
lib/data/analytics.js Outdated Show resolved Hide resolved
"recent": 0,
"total": 0
},
"num_client_audit_attachments_processed": {
Copy link
Member

Choose a reason for hiding this comment

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

Here, I'm less interested in the number of processed attachments and more the number of rows from processed attachments. It'd be great if the name of the metric were something like num_client_audit_rows. We're asking: how big does this table get?

SELECT count(*) FROM client_audits

lib/data/analytics.js Show resolved Hide resolved
lib/data/analytics.js Show resolved Hide resolved
@matthew-white
Copy link
Member

@ktuite, if you wouldn't mind double-checking my queries, that would be excellent. I think I've tried them all against the test server, and there weren't errors, but it'd be great if you could briefly consider whether they will accurately return the metrics of interest.

@ktuite ktuite force-pushed the ktuite/analytics-2023.4 branch 2 times, most recently from da379c8 to 0aaf57b Compare September 11, 2023 18:00
@ktuite ktuite marked this pull request as ready for review September 11, 2023 18:00
@ktuite
Copy link
Member Author

ktuite commented Sep 12, 2023

There's an issue where running/trying to submit the metrics to an ODK server doesn't work. Locally and in prod (on staging), when this runs:

node lib/bin/run-analytics.js

It hangs for a while and then gets this error:

{"sent":false,"message":"Error submitting analytics","error":{"code":"ECONNRESET"}}

It could be an issue with FormData (not data about our forms!)... node 18 has built in FormData now but it may not work the same way with streaming stuff, pipeline, etc. as before. Need to fix/change something probably in external/odk-analytics.js.

@matthew-white
Copy link
Member

I think we could address that issue in a separate PR if that'd make things easier. I doubt it'll be a big change, but it feels like a separate change from the changes to the metrics here.

lib/model/query/analytics.js Outdated Show resolved Hide resolved
lib/model/query/analytics.js Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Tests look great! 💯

metrics.system.num_audits_unprocessed = failedAudits.unprocessed;

// TODO for 2023.4
metrics.system.sso_enabled = 0;// (something)
Copy link
Member

Choose a reason for hiding this comment

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

I think this could come in a follow-up PR once #910 is merged, if that makes things easier.

Copy link
Member

Choose a reason for hiding this comment

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

#910 is merged, so it should be possible to calculate this metric now. 🎉

@ktuite
Copy link
Member Author

ktuite commented Sep 12, 2023

Successfully tested SSO metric by running it in dev backend

make fake-oidc-server
make dev-oidc

and frontend

VUE_APP_OIDC_ENABLED=true npm run dev

and creating a central user to match a mock oidc user.

@ktuite
Copy link
Member Author

ktuite commented Sep 12, 2023

Merging this, but there seems to be a problem with actually sending the metrics: #970

@ktuite ktuite merged commit 2be613d into master Sep 12, 2023
4 checks passed
@ktuite ktuite deleted the ktuite/analytics-2023.4 branch September 12, 2023 18:39
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.

2 participants