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

🐛 Fix missing URL context #3323

Merged
merged 1 commit into from
Feb 5, 2025
Merged

Conversation

amortemousque
Copy link
Collaborator

@amortemousque amortemousque commented Feb 5, 2025

Motivation

When assembling an event for a given timestamp, the view context is found, but the URL context is missing. (cf telemetry PR)

const viewHistoryEntry =  viewHistory.findView(startTime) // get an entry
const urlContext = urlContexts.findUrl(startTime) // get undefined

This happens because both viewHistory and urlContexts rely on createValueHistory, which clears old entries via setInterval. So, the clean-up task of the URL context occurs before the view context.

What happensWhat we expect
  1. urlContexts cleanup task runs first.
  2. Event is assembled, but URL context is missing.
  3. viewHistory cleanup task runs.
  1. Event is assembled after both contexts are cleaned.
  2. urlContexts cleanup task runs
  3. viewHistory cleanup task runs

Changes

  • Update the createValueHistory so that all valueHistory instances clear old entries in the same synchronous task

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@amortemousque amortemousque requested a review from a team as a code owner February 5, 2025 11:23
Copy link
Contributor

@sethfowler-datadog sethfowler-datadog left a comment

Choose a reason for hiding this comment

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

Nice; maintaining a "transactional" model for value histories makes things a lot easier to reason about!

@amortemousque
Copy link
Collaborator Author

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Feb 5, 2025

Devflow running: /to-staging

View all feedbacks in Devflow UI.


2025-02-05 17:05:50 UTC ℹ️ Branch Integration: starting soon, median merge time is 11m20s

Commit 9844f88fbe will soon be integrated into staging-06.


2025-02-05 17:17:41 UTC ℹ️ Branch Integration: This commit was successfully integrated

Commit 9844f88fbe has been merged into staging-06 in merge commit a27107c852.

Check out the triggered pipeline on Gitlab 🦊

If you need to revert this integration, you can use the following command: /code revert-integration -b staging-06

dd-mergequeue bot added a commit that referenced this pull request Feb 5, 2025
Integrated commit sha: 9844f88

Co-authored-by: Aymeric Mortemousque <[email protected]>
@dd-devflow dd-devflow bot added the staging-06 label Feb 5, 2025
@amortemousque amortemousque merged commit a28adf9 into main Feb 5, 2025
20 checks passed
@amortemousque amortemousque deleted the aymeric/fix-missing-url-context branch February 5, 2025 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants