-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(performance): Updates useIndexedSpans to accept an optional fields array #66275
feat(performance): Updates useIndexedSpans to accept an optional fields array #66275
Conversation
Reusing this hook for INP span samples and I want to be able to selectively choose fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I know this hook doesn't have any existing specs, but right now is a really great time to add some 🙏🏻
// TODO: we're using all SpanIndexedFields here, but maybe we should only use what we need? | ||
// Truncate to 20 fields otherwise discover will complain. | ||
const fields = Object.values(SpanIndexedField).slice(0, 20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a todo note here. We need to update this because SpanIndexedField
is eventually going to grow beyond 20 elements and Discover doesn't support querying more than 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you, but IMO it's fine to let Discover complain, rather than truncating the fields, since that's a silent failure! Anyone who doesn't know about the limit would be really confused why fields they're asking for aren't coming back, and everyone should provide the list of fields they want, rather than defaulting to all fields coming back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be mad about a spec here, but doesn't have to be this PR 👍🏻
…ds array (#66275) Updates useIndexedSpans to accept an optional fields array to allow selecting specific fields. Refactors existing call to use new function signature.
Updates useIndexedSpans to accept an optional fields array to allow selecting specific fields.
Refactors existing call to use new function signature.