feat: add apdex support for histograms#1630
feat: add apdex support for histograms#1630sgarfinkel wants to merge 2 commits intohyperdxio:mainfrom
Conversation
🦋 Changeset detectedLatest commit: e4e5f43 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@sgarfinkel is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
| { value: 'any' as const, label: 'Any' }, | ||
| { value: 'none' as const, label: 'None' }, | ||
| { | ||
| group: 'Extra', |
There was a problem hiding this comment.
@teeohhem I mentioned this in discord a while back so wanted to just draw your attention to the UX here. I wasn't sure the best way to display this as a non-aggregation function. This makes it appear as its own group.
There was a problem hiding this comment.
Three things:
- Let's add a top group labeled "Aggregations"
- Let's rename this to "Derived Metrics"
- Let's update the label for now to Apdex (histogram only)
I'd also prefer it if this is disabled or validates into an error state when a non-histogram is selected. Let me know if you need a hand and I can help with some of this.
There was a problem hiding this comment.
Will do! I think I can get at the selected metric type, will have to investigate if that's available via a watch.
dhable
left a comment
dhable
left a comment
There was a problem hiding this comment.
A couple of comments on the query generation.
| valueAlias: TemplatedInput; | ||
| }): WithClauses => [ | ||
| { | ||
| name: 'source', |
There was a problem hiding this comment.
Much of this logic is the same as the source CTE chunk for the aggregate based histogram charts. Maybe we should make this a reusable chunk that we can use in both cases.
The benefit there is that we would have test coverage from two different angles instead of having two chunks that could drift going forward.
| ${timeBucketSelect}, | ||
| ${groupBy ? chSql`[${groupBy}] AS group,` : ''} | ||
| sumForEach(deltas) AS bucket_counts, | ||
| ${{ Float64: threshold }} AS threshold |
There was a problem hiding this comment.
This is a constant value and including in the CTE means it's going to be materialized on every record in the source CTE when it doesn't need to. Could we move this either in-line or at least on the metrics CTE to skip materializing this on the sub-query?
3 participants
Adds apdex support in HyperDX. This currently only works with histograms (and will also work with exponential histograms if those are eventually supported).
Here's the UX:
