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

[ENG-6958][DRAFT] Add pageload logging #10940

Open
wants to merge 5 commits into
base: feature/counter-sushi
Choose a base branch
from

Conversation

opaduchak
Copy link
Contributor

Purpose

Changes

QA Notes

Please make verification statements inspired by your code and what your code touches.

  • Verify
  • Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-6958

@opaduchak opaduchak changed the base branch from develop to feature/counter-sushi January 22, 2025 15:37
Copy link
Member

@felliott felliott left a comment

Choose a reason for hiding this comment

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

Looks good! While CRing I made some changes that I've included here:

  • I renamed the setting var to be more descriptive
  • I updated the base mako template to add the repoid to the global window variables, leaving js loading to the js code. This is similar to how we used to load the old keen code.

It just needs two small changes; one a comment, and the other to fix the strange loading of datacite-tracker. Thank you!

Cheers,
@felliott

@@ -344,6 +344,7 @@ def parent_dir(path):
DATACITE_PASSWORD = None
DATACITE_URL = 'https://mds.datacite.org'
DATACITE_PREFIX = '10.70102' # Datacite's test DOI prefix -- update in production
DATACITE_USAGE_TRACKER_REPO_ID = None
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining what this is for (maybe with a link to https://support.datacite.org/docs/datacite-usage-tracker).

Copy link
Member

Choose a reason for hiding this comment

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

I renamed the setting to DATACITE_USAGE_TRACKER_REPO_ID, but please shorten that to DATACITE_TRACKER_REPO_ID so we're consistent with ember-osf-web.

I said this offline, adding it here for future reference.

@@ -0,0 +1,24 @@
const {Tracker} = require('@datacite/datacite-tracker');
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this to avoid the import troubles w/ datacite-tracker similar to how you did in CenterForOpenScience/ember-osf-web#2496?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one doesn't have import troubles as this is simple javascript, and not typescript as ember is

Copy link
Member

Choose a reason for hiding this comment

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

It imports okay, but it still runs most of the superfluous code in index.js and leaks this message: https://github.com/datacite/datacite-tracker/blob/main/src/index.ts#L30 It looks like we don't need any of the code from index.js, so it would be good if we could bypass that.

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