Skip to content

Fix unit length search #8627

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.

Sign up for GitHub

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

Jump to bottom
Merged
SpennyNDaJets merged 3 commits into from
May 21, 2024
Merged

Fix unit length search #8627

SpennyNDaJets merged 3 commits into from
May 21, 2024

Conversation

@SpennyNDaJets
Copy link
Contributor

@SpennyNDaJets SpennyNDaJets commented May 20, 2024

Summary

Some time related keys have different units. Fix the search to convert to the correct unit:

  • Duration -> ns
  • Length -> ms
  • ActiveLength -> ms
  • Default -> ns

Open question: what is a session's duration and what is its units?

https://www.loom.com/share/f66d69ae0df544f5bb70dcf1a0b8c30a

How did you test this change?

  1. Search for sessions by length with units
  • Correct sessions returned
  1. Search for sessions by active_length with units
  • Correct sessions returned
  1. Search for traces by duration with units
  • Correct traces returned

Are there any deployment considerations?

N/A

Does this work require review from our design team?

N/A

@changeset-bot
Copy link

changeset-bot bot commented May 20, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 41a2eeb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR 
Some errors occurred when validating the changesets config:
The package or glob expression "rrdom" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrdom-nodejs" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb-player" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb-snapshot" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@rrweb/types" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@rrweb/web-extension" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrvideo" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

greptile-apps[bot]
greptile-apps bot reviewed May 20, 2024
View reviewed changes
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Pull Request Summary: Fix unit length search

  • Backend Parser Updates: Updated listener.go to convert time-related keys to the correct units (Duration to ns, Length to ms, ActiveLength to ms, default to ns). Renamed traceAttributeKey to extendedAttributeKey for clarity.
  • Unit Tests: Added new test cases in listener_test.go to ensure correct unit conversions for time-related keys.
  • Frontend Graphing Component: Introduced a default time metric and updated the getTickFormatter function in Graph.tsx to ensure accurate time-based data representation.

Notes:

  • Ensure that all time-related searches and graph representations are consistent with the new unit conversions to avoid discrepancies.

@SpennyNDaJets SpennyNDaJets marked this pull request as ready for review May 20, 2024 16:30
@SpennyNDaJets SpennyNDaJets requested review from a team and ccschmitz and removed request for a team May 20, 2024 16:30
ccschmitz
ccschmitz approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@ccschmitz ccschmitz left a comment

Choose a reason for hiding this comment

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

Code + click test LGTM!

@SpennyNDaJets SpennyNDaJets merged commit 98e61d9 into main May 21, 2024
@SpennyNDaJets SpennyNDaJets deleted the spenny/fix-unit-search branch May 21, 2024 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ccschmitz ccschmitz ccschmitz approved these changes

+1 more reviewer

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SpennyNDaJets @ccschmitz