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

[DR-3476] Include additional fields in DRS getAccessURL Bard logs #1753

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

samanehsan
Copy link
Contributor

@samanehsan samanehsan commented Jul 31, 2024

Jira ticket: https://broadworkbench.atlassian.net/browse/DR-3476

Addresses

Key stakeholders such as the NIH want to know the usage patterns of the data that they are storing. Adding the snapshot name is more informative than using just the snapshot ids. This along with the addition of the billing profile id, dataset id and dataset name allows us to answer questions like, "What are the top X datasets and/or snapshots (of billing profile Y) that users are requesting data from?"

Summary of changes

Add the following fields to the DRS Bard log for getAccessURL (Note these are already available in the snapshot object that gets retrieved from the database, but these fields were not included in the cached snapshot object):

  • snapshot id
  • snapshot name
  • dataset id
  • dataset name
  • billing profile id
  • cloud platform

Testing Strategy

Local testing:

  • Enabled logging to Bard by setting usermetrics.bardBasePath=https://terra-bard-dev.appspot.com in application.properties
  • Ran the TDR server locally in debug mode with a breakpoint in UserMetricsInterceptor (where the call to Bard is made)
  • Ran the drsHacky test to hit the breakpoint
  • Verified that the additional properties get included in the request to get an access url

Add the snapshot name and billing profile id to the
DRS log for getAccessURL. This will allow us to know
which snapshots and billing profiles (sometimes associated
with a specific project) users are requesting data from.
@samanehsan samanehsan marked this pull request as ready for review August 5, 2024 15:44
@samanehsan samanehsan requested review from a team as code owners August 5, 2024 15:44
@samanehsan samanehsan requested review from rushtong and fboulnois and removed request for a team August 5, 2024 15:44
Copy link
Contributor

@okotsopoulos okotsopoulos 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, I left a few asks re: code organization.

Would you mind updating your PR description to share what your testing strategy was for this PR (it's a section in our PR template)? Thanks!

@samanehsan samanehsan force-pushed the se/DR-3476-update-drs-bard-log-info branch from 0ff6555 to ea99ff5 Compare August 5, 2024 18:11
@samanehsan samanehsan force-pushed the se/DR-3476-update-drs-bard-log-info branch from ea99ff5 to 69adc4c Compare August 5, 2024 18:12
Copy link

sonarqubecloud bot commented Aug 5, 2024

@samanehsan
Copy link
Contributor Author

Thanks for the review @okotsopoulos! I made the changes you requested and added a note about how I tested the PR :octocat:

@samanehsan samanehsan merged commit 017163e into develop Aug 5, 2024
13 checks passed
@samanehsan samanehsan deleted the se/DR-3476-update-drs-bard-log-info branch August 5, 2024 20:12
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.

4 participants