Pyroscope: Send start/end with profile types query

[bryanhuhta]

What is this feature?

Adds start/end when making a ProfileTypes query from Pyroscope.

Why do we need this feature?

we need to pass start and end to Pyroscope when asking for profile types in order to accommodate users that may either have not sent profiling data in a long time or have a change what kind profiling data they send. This allows Pyroscope to query the correct time window and return accurate data. Prior to this change, Pyroscope would only return whatever information was in the head block of the ingesters.

Which issue(s) does this PR fix?:

Related: grafana/pyroscope#2230 (Fixes, but I don't want the auto close).

Special notes for your reviewer:

There is a PR to improve the ProfileTypes API and correctly handle start/end parameters. (grafana/pyroscope#2617). This PR needs to be merged first.

Please check that:

• It works as expected from a user's perspective.
• If this is a pre-GA feature, it is behind a feature toggle.
• The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

[joey-grafana]

Appears to work nicely 👍

Will need to fix some tests though!

[joey-grafana]

@bryanhuhta you will still need to add backend code to send start/end to Pyroscope

[bryanhuhta]

@bryanhuhta you will still need to add backend code to send start/end to Pyroscope

I added backend code in Pyroscope to handle start/end in grafana/pyroscope#2617. Is there any other backend code I might need to consider?

I got distracted with a few other things last week, I'm circling back now to get tests here fixed up and get this merged in.

[joey-grafana]

Hey yeah so we are getting the start and end time on the frontend and it seems to be already done in Pyroscope API, but the start/end from the frontend is not actually being used in the Pyroscope API call i.e. the start/end needs to be added to the Pyroscope backend code.

[joey-grafana]

We are requesting the ProfileTypes API a lot of times, is there a particular reason for this or is the UI re-rendering a lot?

[joey-grafana]

Not sure why we're going back a day here but providing start/end of 0 in <QueryEditor />?

[bryanhuhta]

I copy-pasted this from the <LabelRow /> part, assuming we wanted similar behavior.

grafana/public/app/plugins/datasource/grafana-pyroscope-datasource/VariableQueryEditor.tsx

Lines 74 to 85 in 8e01744

	<LabelRow
	value={props.query.labelName}
	datasource={props.datasource}
	profileTypeId={props.query.profileTypeId}
	onChange={(val) => {
	if (props.query.type === 'labelValue') {
	props.onChange({ ...props.query, labelName: val });
	}
	}}
	from={props.range?.from.valueOf() || Date.now().valueOf() - 1000 * 60 * 60 * 24}
	to={props.range?.to.valueOf() || Date.now().valueOf()}
	/>

[bryanhuhta]

We are requesting the ProfileTypes API a lot of times, is there a particular reason for this or is the UI re-rendering a lot?

This is a regression I caused. I suspect it's stemming from this function being changed to take start/end:

grafana/public/app/plugins/datasource/grafana-pyroscope-datasource/QueryEditor/ProfileTypesCascader.tsx

Lines 76 to 87 in 43da2ba

	export function useProfileTypes(datasource: PyroscopeDataSource, start: number, end: number) {
	const [profileTypes, setProfileTypes] = useState<ProfileTypeMessage[]>();
	useEffect(() => {
	(async () => {
	const profileTypes = await datasource.getProfileTypes(start, end);
	setProfileTypes(profileTypes);
	})();
	}, [datasource, start, end]);
	return profileTypes;
	}

These start/end values might be props changing by callers, causing the useEffect to be called several extra times.

grafana/public/app/plugins/datasource/grafana-pyroscope-datasource/QueryEditor/QueryEditor.tsx

Line 28 in 43da2ba

const profileTypes = useProfileTypes(datasource, range?.from.valueOf() || 0, range?.to.valueOf() || 0);

grafana/public/app/plugins/datasource/grafana-pyroscope-datasource/VariableQueryEditor.tsx

Line 139 in 43da2ba

const profileTypes = useProfileTypes(props.datasource, props.from, props.to);

Though, I'm not a React expert and that's only my current working theory. I do need to fix this before this PR is OK to merge.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[aocenas]

@bryanhuhta if the rerenders are because of the time range it's probably because if you use range now-1h <-> now for example then every time something rerenders the times also change and so you end up with this being called any time anything else changes. So I would say this should be memorized in a way where the time we use isn't millisecond precision as we probably don't need to refresh that on every millisecond anyway. So it could be truncated to most recent minute or something like that.

[bryanhuhta]

After 97a55a0, this PR should be ready to go. We no longer make a series of queries as the time range changes slightly during page load.

[joey-grafana]

Thanks for fixing those remaining issues! LGTM 👍