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

CORE-188: batch upsert tracing #3147

Merged
merged 4 commits into from
Dec 6, 2024
Merged

Conversation

davidangb
Copy link
Contributor

@davidangb davidangb commented Dec 5, 2024

Ticket: https://broadworkbench.atlassian.net/browse/CORE-188

The batchUpsert API is used by TSV uploads, among others.

In this PR:

  • fixes tracing for the batchUpsert API by passing the tracing context down to submethods
  • fixes a couple places where trace subspans were started eagerly and thus didn't show properly in the trace timeline
  • adds some tracing subspans for more detail

Before this PR

  • the getActiveEntities and saveAction subspans appear to start at the same time, which is incorrect
  • note the lack of detail under the LocalEntityProvider.batchUpdateEntitiesImpl subspan
    Screenshot 05-12-2024 at 15 20 (3)

After this PR:

  • much more detail under the LocalEntityProvider.batchUpdateEntitiesImpl subspan
  • timeline for getActiveEntities and saveAction is more accurate
    Screenshot 05-12-2024 at 15 45 (1)

PR checklist

  • Include the JIRA issue number in the PR description and title
  • Make sure Swagger is updated if API changes
    • ...and Orchestration's Swagger too!
  • If you changed anything in model/, then you should publish a new official rawls-model and update rawls-model in Orchestration's dependencies.
  • Get two thumbsworth of PR review
  • Verify all tests go green, including CI tests
  • Squash commits and merge to develop (branches are automatically deleted after merging)
  • Inform other teams of any substantial changes via Slack and/or email

for {
_ <- traceDBIOWithParent("insertIntoScratchFunction", span)(_ => insertIntoScratchFunction())
numUpdates <- traceDBIOWithParent("updateInMasterAction", span)(_ => updateInMasterAction())
} yield numUpdates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the change from andThen to for ... yield ensures the second "updateInMasterAction" trace does not start eagerly

}
}

traceDBIOWithParent("saveAction", localContext)(_ => saveAction)
saveAction
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moving the trace wrapper up ensures it does not start eagerly

@davidangb davidangb marked this pull request as ready for review December 5, 2024 21:45
@davidangb davidangb requested a review from a team as a code owner December 5, 2024 21:45
@davidangb davidangb requested review from marctalbott and trholdridge and removed request for a team December 5, 2024 21:45
@davidangb davidangb merged commit 4def381 into develop Dec 6, 2024
29 checks passed
@davidangb davidangb deleted the da_CORE-188_batchUpsertTracing branch December 6, 2024 15:03
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.

3 participants