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

Use text for relative timestamps and numbers for absolute #618

Merged
merged 1 commit into from
Feb 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ui/src/components/graphs/DurationGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
Series,
Timeseries,
} from '../../proto/objectives/v1alpha1/objectives_pb'
import {selectTimeRange} from './selectTimeRange'

interface DurationGraphProps {
client: PromiseClient<typeof ObjectiveService>
Expand All @@ -23,6 +24,7 @@ interface DurationGraphProps {
from: number
to: number
uPlotCursor: uPlot.Cursor
updateTimeRange: (min: number, max: number, absolute: boolean) => void
target: number
latency: number | undefined
}
Expand All @@ -34,6 +36,7 @@ const DurationGraph = ({
from,
to,
uPlotCursor,
updateTimeRange,
target,
latency,
}: DurationGraphProps): JSX.Element => {
Expand Down Expand Up @@ -189,6 +192,9 @@ const DurationGraph = ({
v.map((v: number) => formatDuration(v * 1000)),
},
],
hooks: {
setSelect: [selectTimeRange(updateTimeRange)],
},
}}
data={durations}
/>
Expand Down
6 changes: 6 additions & 0 deletions ui/src/components/graphs/ErrorBudgetGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {PromiseClient} from '@bufbuild/connect-web'
import {ObjectiveService} from '../../proto/objectives/v1alpha1/objectives_connectweb'
import {GraphErrorBudgetResponse} from '../../proto/objectives/v1alpha1/objectives_pb'
import {Timestamp} from '@bufbuild/protobuf'
import {selectTimeRange} from './selectTimeRange'

interface ErrorBudgetGraphProps {
client: PromiseClient<typeof ObjectiveService>
Expand All @@ -20,6 +21,7 @@ interface ErrorBudgetGraphProps {
from: number
to: number
uPlotCursor: uPlot.Cursor
updateTimeRange: (min: number, max: number, absolute: boolean) => void
}

const ErrorBudgetGraph = ({
Expand All @@ -29,6 +31,7 @@ const ErrorBudgetGraph = ({
from,
to,
uPlotCursor,
updateTimeRange,
}: ErrorBudgetGraphProps): JSX.Element => {
const targetRef = useRef() as React.MutableRefObject<HTMLDivElement>

Expand Down Expand Up @@ -200,6 +203,9 @@ const ErrorBudgetGraph = ({
values: (uplot: uPlot, v: number[]) => v.map((v: number) => `${v.toFixed(2)}%`),
},
],
hooks: {
setSelect: [selectTimeRange(updateTimeRange)],
},
}}
data={samples}
/>
Expand Down
6 changes: 6 additions & 0 deletions ui/src/components/graphs/ErrorsGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {PromiseClient} from '@bufbuild/connect-web'
import {ObjectiveService} from '../../proto/objectives/v1alpha1/objectives_connectweb'
import {Timestamp} from '@bufbuild/protobuf'
import {GraphErrorsResponse, Series} from '../../proto/objectives/v1alpha1/objectives_pb'
import {selectTimeRange} from './selectTimeRange'

interface ErrorsGraphProps {
client: PromiseClient<typeof ObjectiveService>
Expand All @@ -20,6 +21,7 @@ interface ErrorsGraphProps {
from: number
to: number
uPlotCursor: uPlot.Cursor
updateTimeRange: (min: number, max: number, absolute: boolean) => void
Copy link
Contributor

@manojVivek manojVivek Feb 20, 2023

Choose a reason for hiding this comment

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

I guess, for less duplication and more type rigidity, there can be a base type called GraphProps with fields like from, to, updateTimeRange and that type can be extended in these three components to add their own fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense, yes. Given this is a PR to fix a bug I won't address this now. We should definitely do this later!

}

const ErrorsGraph = ({
Expand All @@ -30,6 +32,7 @@ const ErrorsGraph = ({
from,
to,
uPlotCursor,
updateTimeRange,
}: ErrorsGraphProps): JSX.Element => {
const targetRef = useRef() as React.MutableRefObject<HTMLDivElement>

Expand Down Expand Up @@ -157,6 +160,9 @@ const ErrorsGraph = ({
values: (uplot: uPlot, v: number[]) => v.map((v: number) => `${v}%`),
},
],
hooks: {
setSelect: [selectTimeRange(updateTimeRange)],
},
}}
data={errors}
/>
Expand Down
6 changes: 6 additions & 0 deletions ui/src/components/graphs/RequestsGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {PromiseClient} from '@bufbuild/connect-web'
import {ObjectiveService} from '../../proto/objectives/v1alpha1/objectives_connectweb'
import {GraphRateResponse, Series} from '../../proto/objectives/v1alpha1/objectives_pb'
import {Timestamp} from '@bufbuild/protobuf'
import {selectTimeRange} from './selectTimeRange'

interface RequestsGraphProps {
client: PromiseClient<typeof ObjectiveService>
Expand All @@ -19,6 +20,7 @@ interface RequestsGraphProps {
from: number
to: number
uPlotCursor: uPlot.Cursor
updateTimeRange: (min: number, max: number, absolute: boolean) => void
}

const RequestsGraph = ({
Expand All @@ -28,6 +30,7 @@ const RequestsGraph = ({
from,
to,
uPlotCursor,
updateTimeRange,
}: RequestsGraphProps): JSX.Element => {
const targetRef = useRef() as React.MutableRefObject<HTMLDivElement>

Expand Down Expand Up @@ -152,6 +155,9 @@ const RequestsGraph = ({
},
},
},
hooks: {
setSelect: [selectTimeRange(updateTimeRange)],
},
}}
data={requests}
/>
Expand Down
10 changes: 10 additions & 0 deletions ui/src/components/graphs/selectTimeRange.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import uPlot from 'uplot'

export const selectTimeRange =
(updateTimeRange: (min: number, max: number, absolute: boolean) => void) => (u: uPlot) => {
if (u.select.width > 0) {
const min = u.posToVal(u.select.left, 'x')
const max = u.posToVal(u.select.left + u.select.width, 'x')
updateTimeRange(Math.floor(min * 1000), Math.floor(max * 1000), true)
}
}
46 changes: 40 additions & 6 deletions ui/src/pages/Detail.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
hasObjectiveType,
latencyTarget,
ObjectiveType,
parseDuration,
renderLatencyTarget,
} from '../App'
import Navbar from '../components/Navbar'
Expand Down Expand Up @@ -70,11 +71,26 @@ const Detail = () => {

const name: string = labels[MetricName]

let to: number = Date.now()
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, to, from parsing logic and updateTimeRange callback can be extracted into a useTimeRange hook, so that the timeRange parsing and setting logic would be in one place inside that hook without getting spilled into this component.

But I guess since this is the only page that uses timeRange, this hasn't surfaced as a problem.

const toQuery = query.get('to')
const to = toQuery != null ? parseInt(toQuery) : Date.now()
if (toQuery !== null) {
if (!toQuery.includes('now')) {
to = parseInt(toQuery)
}
}

let from: number = to - 60 * 60 * 1000
const fromQuery = query.get('from')
const from = fromQuery != null ? parseInt(fromQuery) : to - 3600 * 1000
if (fromQuery !== null) {
if (fromQuery.includes('now')) {
const duration = parseDuration(fromQuery.substring(4)) // omit first 4 chars: `now-`
if (duration !== null) {
from = to - duration
}
} else {
from = parseInt(fromQuery)
}
}

document.title = `${name} - Pyrra`

Expand Down Expand Up @@ -159,12 +175,26 @@ const Detail = () => {
}, [getObjective, getObjectiveStatus])

const updateTimeRange = useCallback(
(from: number, to: number) => {
navigate(`/objectives?expr=${expr}&grouping=${groupingExpr ?? ''}&from=${from}&to=${to}`)
(from: number, to: number, absolute: boolean) => {
let fromStr = from.toString()
let toStr = to.toString()
if (!absolute) {
fromStr = `now-${formatDuration(to - from)}`
toStr = 'now'
}
navigate(
`/objectives?expr=${expr}&grouping=${groupingExpr ?? ''}&from=${fromStr}&to=${toStr}`,
)
},
[navigate, expr, groupingExpr],
)

const updateTimeRangeSelect = (min: number, max: number, absolute: boolean) => {
// when selecting time ranges with the mouse we want to disable the auto refresh
setAutoReload(false)
updateTimeRange(min, max, absolute)
}

const duration = to - from
const interval = intervalFromDuration(duration)

Expand All @@ -173,7 +203,7 @@ const Detail = () => {
const id = setInterval(() => {
const newTo = Date.now()
const newFrom = newTo - duration
updateTimeRange(newFrom, newTo)
updateTimeRange(newFrom, newTo, false)
}, interval)

return () => {
Expand All @@ -185,7 +215,7 @@ const Detail = () => {
const handleTimeRangeClick = (t: number) => () => {
const to = Date.now()
const from = to - t
updateTimeRange(from, to)
updateTimeRange(from, to, false)
}

if (objectiveError !== '') {
Expand Down Expand Up @@ -450,6 +480,7 @@ const Detail = () => {
from={from}
to={to}
uPlotCursor={uPlotCursor}
updateTimeRange={updateTimeRangeSelect}
/>
</Col>
</Row>
Expand All @@ -465,6 +496,7 @@ const Detail = () => {
from={from}
to={to}
uPlotCursor={uPlotCursor}
updateTimeRange={updateTimeRangeSelect}
/>
</Col>
<Col
Expand All @@ -479,6 +511,7 @@ const Detail = () => {
from={from}
to={to}
uPlotCursor={uPlotCursor}
updateTimeRange={updateTimeRangeSelect}
/>
</Col>
{objectiveType === ObjectiveType.Latency ? (
Expand All @@ -490,6 +523,7 @@ const Detail = () => {
from={from}
to={to}
uPlotCursor={uPlotCursor}
updateTimeRange={updateTimeRangeSelect}
target={objective.target}
latency={latencyTarget(objective)}
/>
Expand Down