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

Exemplars: Add exemplar tooltip config #73350

Closed
wants to merge 4 commits into from

Conversation

aishyandapalli
Copy link
Contributor

This PR adds a new feature to the Options Pane for PanelEditor. It adds a new section called Exemplar Configuration where you can order, hide or unhide different exemplar labels.

This feature is needed because when there are too many labels for Exemplars, the tooltip is very hard to be readable and we want to provide a configuration for multiple users where they can configure by themselves.

@aishyandapalli aishyandapalli requested review from a team and Eve832 as code owners August 16, 2023 20:25
@aishyandapalli aishyandapalli requested review from tskarhed, eledobleefe and mckn and removed request for a team August 16, 2023 20:25
@aishyandapalli
Copy link
Contributor Author

@gtk-grafana Please check this new PR for the exemplar configuration changes. Had to close the old PR due to some issues.

Old PR is #72765

@aishyandapalli aishyandapalli changed the title Exemplars config [v10.2.x] Exemplars: Add exemplar configuration to Options Pane to order, hide or unhide exemplar labels on tooltip Aug 16, 2023
@gtk-grafana
Copy link
Contributor

gtk-grafana commented Aug 17, 2023

Thanks @aishyandapalli. I'll try to find some time later today or tomorrow to check it out, but I see that some frontend tests are failing in drone.

yarn run ci:test-frontend in the project root will run the test suite that is failing, in your local environment.

@aishyandapalli
Copy link
Contributor Author

@gtk-grafana I will fix those issues and update the PR

@aishyandapalli
Copy link
Contributor Author

@gtk-grafana I fixed the errors for test cases related to exemplars but, I am seeing 2 more errors unrelated to the changes in this PR and I found those failing on the main branch too. Any suggestion would help

@gtk-grafana
Copy link
Contributor

Heatmap functionality is working for me now

image

And seeing everything in a single collapsable section now.
This looks good to me from a functionality/user perspective, but we'll need @grafana/observability-metrics and @grafana/dataviz-squad to review the changes to their code bases before final approval.

@bohandley from metrics has agreed to try to find some time to help out and keep this review moving forward while I'm OOO until mid September.

@aishyandapalli
Copy link
Contributor Author

@gtk-grafana Thanks a lot for helping with this. I resolved the conflicts as well. Will wait for further comments.

@bohandley Thanks in advance

@nmarrs nmarrs added this to the 10.2.x milestone Aug 22, 2023
@nmarrs nmarrs changed the title [v10.2.x] Exemplars: Add exemplar configuration to Options Pane to order, hide or unhide exemplar labels on tooltip Exemplars: Add exemplar configuration to Options Pane to order, hide or unhide exemplar labels on tooltip Aug 22, 2023
@nmarrs nmarrs changed the title Exemplars: Add exemplar configuration to Options Pane to order, hide or unhide exemplar labels on tooltip Exemplars: Add exemplar tooltip config Aug 22, 2023
@imatwawana
Copy link
Collaborator

@nmarrs - Do the Heatmap and Time series docs also need to be updated to describe this addition?

@nmarrs
Copy link
Contributor

nmarrs commented Aug 22, 2023

@imatwawana - yes we should update docs for this feature, in my opinion that can be done in a follow up PR though

@imatwawana
Copy link
Collaborator

@imatwawana - yes we should update docs for this feature, in my opinion that can be done in a follow up PR though

I've opened an issue so it doesn't get lost in the fray. It's assigned to me for the purposes of docs triage, but feel free to grab it.

@nmarrs nmarrs modified the milestone: 10.2.x Aug 23, 2023
public/app/plugins/panel/heatmap/module.tsx Outdated Show resolved Hide resolved
tooltip="Add exemplar labels to be displayed on exemplar tooltip. Note that the order of labels added is also important and the same order is displayed on the exemplar tooltip"
>
<TagsInput placeholder="trace_id" tags={labels} onChange={(ls) => setExemplarLabels(ls)} />
</InlineField>
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed a couple of UI inconsistencies here:

  1. The UI doesn't scale well, if a user adds too many labels the tags / input go off of the page
    design breaks if too many tags are added

  2. The input width is tied to the number of tags (ideally it is just constant)
    length of input is tied to number of tags - should just be static

<TagsInput placeholder="trace_id" tags={labels} onChange={(ls) => setExemplarLabels(ls)} />
</InlineField>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

UX wise I found it hard to remember all of the available exemplar labels - as soon as I added one tag the entire list was hidden. Combining the above feedback with this thought perhaps the best UI / UX for this feature might be the following:

Under the Tooltip panel edit options - if exemplar labels exist in the heatmap / time series render the ExemplarLabelEditor

The label editor could be a list of the available exemplar labels, each with a button to hide / unhide + have the ability to be dragged / dropped into any order (assuming it is possible to display this list in the editor context (cc @itsmylife / @bohandley)

What do you think of this approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like your suggestion for the approach @nmarrs. This is similar to the organize fields transformation, where each field is listed vertically, each field can be hidden and the order can be changed. (I would suggest not using the 'rename field' functionality of this UI though because it doesn't seem in scope for the work)

Screenshot 2023-08-23 at 8 50 50 AM

Here is the code for how this UI is implemented

interface DraggableFieldProps {
fieldName: string;
renamedFieldName?: string;
index: number;
visible: boolean;
onToggleVisibility: (fieldName: string, isVisible: boolean) => void;
onRenameField: (from: string, to: string) => void;
}
const DraggableFieldName = ({
fieldName,
renamedFieldName,
index,
visible,
onToggleVisibility,
onRenameField,
}: DraggableFieldProps) => {
const styles = useStyles2(getFieldNameStyles);
return (
<Draggable draggableId={fieldName} index={index}>
{(provided) => (
<div className="gf-form-inline" ref={provided.innerRef} {...provided.draggableProps}>
<div className="gf-form gf-form--grow">
<div className="gf-form-label gf-form-label--justify-left width-30">
<Icon
name="draggabledots"
title="Drag and drop to reorder"
size="lg"
className={styles.draggable}
{...provided.dragHandleProps}
/>
<IconButton
className={styles.toggle}
size="md"
name={visible ? 'eye' : 'eye-slash'}
onClick={() => onToggleVisibility(fieldName, visible)}
tooltip={visible ? 'Disable' : 'Enable'}
/>
<span className={styles.name} title={fieldName}>
{fieldName}
</span>
</div>
<Input
className="flex-grow-1"
defaultValue={renamedFieldName || ''}
placeholder={`Rename ${fieldName}`}
onBlur={(event) => onRenameField(fieldName, event.currentTarget.value)}
/>
</div>
</div>
)}
</Draggable>
);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmarrs @bohandley I like this approach too but, how do we get the list of exemplar fields in ExemplarLabelEditor? Is it already available in the props? I don't see it available in the context object provided to ExemplarLabelEditor. Please suggest

Copy link
Contributor

Choose a reason for hiding this comment

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

@nmarrs it doesn't look like the PanelPlugins currently have any access to the exemplars/annotations, so showing the potential options isn't currently possible.

Do you know how we could get exemplars piped up to these components?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @gtk-grafana / @aishyandapalli - sorry for the delay in response here - this week is pretty slammed catching up from 2 weeks of PTO. I will take a closer look at this on Monday next week, off the top of my head I am not sure if panel plugins have easy access to the list of exemplars / annotations but there may be a way to retrieve this info

Copy link
Contributor

@nmarrs nmarrs Oct 9, 2023

Choose a reason for hiding this comment

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

Hi @aishyandapalli - it appears that we store exemplars as part of the panel's data in the annotations property where the field name is exemplar. See where we pass exemplars to the exemplar plugin in the timeseries panel module file

I'm pretty sure there is a way to access the panel's data from the runtime but with a a little digging I was unable to find the correct helper function 🤔 @gtk-grafana / @leeoniya / @ryantxu any thoughts here?

i.e. data.annotations

Screenshot 2023-10-09 at 9 36 51 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmarrs Yeah, even I checked the values for data.annotations for panel but, couldn't find the data in the editor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmarrs Any update on this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @aishyandapalli - sorry for the long delay here. Thank you so much for this contribution and for bringing up this use case to us. Because you brought this to our attention we realized that we have actually a larger problem with series data frames and annotation data frames which we are going to work to fix.

At this point in time we believe that this use case could be handled in a more scalable way through modifying transforms (i.e. use the existing organize fields transform but with annotation data frames vs series data) (see this issue for more context). We are still working on creating a POC / verifying this approach so for the time being we will leave this PR open in case the more scalable approach isn't viable. Please follow the linked issue to see updates on this matter :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmarrs Thanks a lot for getting back with detailed information. I will wait and keep an eye on the issue :)

} else {
for (let i in exemplarLabels) {
const label = exemplarLabels[i];
const field = dataFrame.fields.find((field) => field.name === label);
Copy link
Contributor

Choose a reason for hiding this comment

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

@itsmylife can exemplar labels be changed via an override etc? We have ran into issues with this before. It may be safest to change this out with getFieldDisplayName(field) vs just field.name as these can in theory be different (see #73332 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

Exemplar labels cannot be changed via an override nor any other UI in the Prometheus ds. Agreed, getFieldDisplayName(field) is preferred because as you state, they can be different.

@aishyandapalli
Copy link
Contributor Author

@nmarrs Thanks a lot for your review. I will address these issues and update the PR

@lukasztyrala
Copy link
Member

Hi @bohandley and @nmarrs, it is hard for me to track the current state of changes, so the below comments might not be relevant, but:

  1. Having Labels as a title and Labels as an input label seems redundant.
  2. Placeholder shows trace_id, so if the input field accepts only trace_id, the input label could be Trace IDs for more clarity.
  3. Because exemplars are rendered on visualization, color plays a special role (e.g., identifies series, value ranges, etc.). If we use tag pills with various colors, people will expect that a specific trace_id tag will be rendered using the same color as a tag.
  4. It would be great if the input field provided some autocomplete – maybe a song of the future or not relevant here.
  5. I agree that people should have control over the order of the exemplars.
  6. What happened to the color selection for trace_ids? :-)

@aishyandapalli
Copy link
Contributor Author

Thank you all for the comments. I will address these comments and update the PR by end of this week.

Copy link
Contributor

github-actions bot commented Dec 9, 2023

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!

@github-actions github-actions bot added the stale Issue with no recent activity label Dec 9, 2023
Copy link
Contributor

This pull request has been automatically closed because it has not had activity in the last 2 weeks. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Dec 24, 2023
@zserge zserge modified the milestones: 10.2.x, 10.2.4 Jan 29, 2024
@nmarrs
Copy link
Contributor

nmarrs commented Jan 30, 2024

This PR was closed in favor of the solution found in this PR: #77842

Thank you @aishyandapalli for your contribution and inspiration on implementing this functionality! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants